diff --git a/changelog.d/fixes/9088-skills-routes-error-sanitization.md b/changelog.d/fixes/9088-skills-routes-error-sanitization.md new file mode 100644 index 0000000000..7962220e6c --- /dev/null +++ b/changelog.d/fixes/9088-skills-routes-error-sanitization.md @@ -0,0 +1 @@ +- **fix(api/skills):** the `/api/skills/**` routes now run caught error messages through `sanitizeErrorMessage` before returning them, so a filesystem failure no longer leaks an absolute path (e.g. `/home//.omniroute/skills/...`) to the client; the `{ error: string }` response shape is preserved for the dashboard ([#9088](https://github.com/diegosouzapw/OmniRoute/pull/9088)) diff --git a/src/app/api/skills/[id]/route.ts b/src/app/api/skills/[id]/route.ts index 8a9aa82e7c..7bad90e118 100644 --- a/src/app/api/skills/[id]/route.ts +++ b/src/app/api/skills/[id]/route.ts @@ -4,6 +4,7 @@ import { skillRegistry } from "@/lib/skills/registry"; import { z } from "zod"; import { validateBody, isValidationFailure } from "@/shared/validation/helpers"; import { requireManagementAuth } from "@/lib/api/requireManagementAuth"; +import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; const updateSkillSchema = z.object({ enabled: z.boolean().optional(), @@ -22,7 +23,7 @@ export async function DELETE(_request: Request, props: { params: Promise<{ id: s } return NextResponse.json({ success: true }); } catch (err: unknown) { - const error = err instanceof Error ? err.message : String(err); + const error = sanitizeErrorMessage(err instanceof Error ? err.message : String(err)); return NextResponse.json({ error }, { status: 500 }); } } @@ -71,7 +72,7 @@ export async function PUT(request: Request, props: { params: Promise<{ id: strin mode: validation.data.mode, }); } catch (err: unknown) { - const error = err instanceof Error ? err.message : String(err); + const error = sanitizeErrorMessage(err instanceof Error ? err.message : String(err)); return NextResponse.json({ error }, { status: 500 }); } } diff --git a/src/app/api/skills/executions/route.ts b/src/app/api/skills/executions/route.ts index 93d7708a3c..d2ef943e62 100644 --- a/src/app/api/skills/executions/route.ts +++ b/src/app/api/skills/executions/route.ts @@ -4,6 +4,7 @@ import { parsePaginationParams, buildPaginatedResponse } from "@/shared/types/pa import { z } from "zod"; import { validateBody, isValidationFailure } from "@/shared/validation/helpers"; import { isAuthenticated } from "@/shared/utils/apiAuth"; +import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; export async function GET(request: Request) { if (!(await isAuthenticated(request))) { @@ -21,7 +22,7 @@ export async function GET(request: Request) { ); return NextResponse.json(buildPaginatedResponse(executions, total, params)); } catch (err: unknown) { - const error = err instanceof Error ? err.message : String(err); + const error = sanitizeErrorMessage(err instanceof Error ? err.message : String(err)); return NextResponse.json({ error }, { status: 500 }); } } @@ -51,7 +52,7 @@ export async function POST(request: Request) { }); return NextResponse.json({ execution }); } catch (err: unknown) { - const error = err instanceof Error ? err.message : String(err); + const error = sanitizeErrorMessage(err instanceof Error ? err.message : String(err)); if (error.includes("disabled")) { return NextResponse.json({ error }, { status: 503 }); } diff --git a/src/app/api/skills/install/route.ts b/src/app/api/skills/install/route.ts index 256f262040..540ee02e56 100644 --- a/src/app/api/skills/install/route.ts +++ b/src/app/api/skills/install/route.ts @@ -3,6 +3,7 @@ import { z } from "zod"; import { skillRegistry } from "@/lib/skills/registry"; import { validateBody, isValidationFailure } from "@/shared/validation/helpers"; import { requireManagementAuth } from "@/lib/api/requireManagementAuth"; +import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; const installManifestSchema = z.object({ name: z.string().min(1).max(100), @@ -44,7 +45,7 @@ export async function POST(request: Request) { return NextResponse.json({ success: true, id: skill.id }); } catch (err: unknown) { - const error = err instanceof Error ? err.message : String(err); + const error = sanitizeErrorMessage(err instanceof Error ? err.message : String(err)); return NextResponse.json({ error }, { status: 500 }); } } diff --git a/src/app/api/skills/marketplace/install/route.ts b/src/app/api/skills/marketplace/install/route.ts index f05ec58044..1690e2b686 100644 --- a/src/app/api/skills/marketplace/install/route.ts +++ b/src/app/api/skills/marketplace/install/route.ts @@ -5,6 +5,7 @@ import { skillRegistry } from "@/lib/skills/registry"; import { getSkillsProviderSetting } from "@/lib/skills/providerSettings"; import { isAuthenticated } from "@/shared/utils/apiAuth"; +import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; const marketplaceInstallSchema = z.object({ name: z.string().min(1).max(64), @@ -53,7 +54,7 @@ export async function POST(request: Request) { return NextResponse.json({ success: true, id: skill.id }); } catch (err: unknown) { - const error = err instanceof Error ? err.message : String(err); + const error = sanitizeErrorMessage(err instanceof Error ? err.message : String(err)); return NextResponse.json({ error }, { status: 500 }); } } diff --git a/src/app/api/skills/marketplace/route.ts b/src/app/api/skills/marketplace/route.ts index e0ae1fa146..7a9d197493 100644 --- a/src/app/api/skills/marketplace/route.ts +++ b/src/app/api/skills/marketplace/route.ts @@ -2,6 +2,7 @@ import { NextResponse } from "next/server"; import { getSettings } from "@/lib/db/settings"; import { isAuthenticated } from "@/shared/utils/apiAuth"; import { getSkillsProviderSetting } from "@/lib/skills/providerSettings"; +import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; const POPULAR_BY_PROVIDER = { skillsmp: ["web-search", "file-reader", "sql-assistant", "devops-helper", "docs-assistant"], @@ -55,7 +56,7 @@ export async function GET(request: Request) { const data = await res.json(); return NextResponse.json({ skills: data.data?.skills || data.skills || [] }); } catch (err: unknown) { - const error = err instanceof Error ? err.message : String(err); + const error = sanitizeErrorMessage(err instanceof Error ? err.message : String(err)); return NextResponse.json({ error }, { status: 500 }); } } diff --git a/src/app/api/skills/route.ts b/src/app/api/skills/route.ts index 2a0c6a26d5..ce5bc98f09 100644 --- a/src/app/api/skills/route.ts +++ b/src/app/api/skills/route.ts @@ -4,6 +4,7 @@ import { parsePaginationParams, buildPaginatedResponse } from "@/shared/types/pa import { getSkillsProviderSetting } from "@/lib/skills/providerSettings"; import { requireManagementAuth } from "@/lib/api/requireManagementAuth"; import { matchesSearch } from "@/shared/utils/turkishText"; +import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; const POPULAR_BY_PROVIDER = { skillsmp: ["web-search", "file-reader", "sql-assistant", "devops-helper", "docs-assistant"], @@ -56,7 +57,7 @@ export async function GET(request?: Request) { popularDefaults: POPULAR_BY_PROVIDER[provider], }); } catch (err: unknown) { - const error = err instanceof Error ? err.message : String(err); + const error = sanitizeErrorMessage(err instanceof Error ? err.message : String(err)); return NextResponse.json({ error }, { status: 500 }); } } diff --git a/src/app/api/skills/skillssh/install/route.ts b/src/app/api/skills/skillssh/install/route.ts index cb3288e2a5..84707c0e73 100644 --- a/src/app/api/skills/skillssh/install/route.ts +++ b/src/app/api/skills/skillssh/install/route.ts @@ -5,6 +5,7 @@ import { skillRegistry } from "@/lib/skills/registry"; import { isAuthenticated } from "@/shared/utils/apiAuth"; import { fetchSkillMd } from "@/lib/skills/skillssh"; import { getSkillsProviderSetting } from "@/lib/skills/providerSettings"; +import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; const skillsshInstallSchema = z.object({ name: z.string().min(1).max(64), @@ -55,7 +56,7 @@ export async function POST(request: Request) { return NextResponse.json({ success: true, id: skill.id }); } catch (err: unknown) { - const error = err instanceof Error ? err.message : String(err); + const error = sanitizeErrorMessage(err instanceof Error ? err.message : String(err)); return NextResponse.json({ error }, { status: 500 }); } } diff --git a/src/app/api/skills/skillssh/route.ts b/src/app/api/skills/skillssh/route.ts index ab587fddef..5ceffbfe69 100644 --- a/src/app/api/skills/skillssh/route.ts +++ b/src/app/api/skills/skillssh/route.ts @@ -1,6 +1,7 @@ import { NextResponse } from "next/server"; import { isAuthenticated } from "@/shared/utils/apiAuth"; import { searchSkillsSh } from "@/lib/skills/skillssh"; +import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; export async function GET(request: Request) { if (!(await isAuthenticated(request))) { @@ -22,7 +23,7 @@ export async function GET(request: Request) { })), }); } catch (err: unknown) { - const error = err instanceof Error ? err.message : String(err); + const error = sanitizeErrorMessage(err instanceof Error ? err.message : String(err)); return NextResponse.json({ error }, { status: 500 }); } } diff --git a/tests/unit/skills-routes-error-sanitization.test.ts b/tests/unit/skills-routes-error-sanitization.test.ts new file mode 100644 index 0000000000..083ef363cc --- /dev/null +++ b/tests/unit/skills-routes-error-sanitization.test.ts @@ -0,0 +1,73 @@ +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"; + +// The /api/skills/** routes returned `{ error: err.message }` verbatim on a 500, +// leaking absolute filesystem paths (skillRegistry writes to disk, so EACCES/ENOSPC +// surface a real path). These routes are consumed by the dashboard as `{ error: string }` +// (OmniMarketplaceTab.tsx, OmniSkillsPageClient.tsx), so the fix keeps the string shape +// and only routes the message through sanitizeErrorMessage (Hard Rule #12: no path/stack leak). +// +// mock.module is unreliable under this tsx/ESM + node:test runner (see +// instrumentation-warm-catalog-cache.test.ts), so the singleton method is monkey-patched. + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-skills-sanitize-")); +const ORIGINAL_DATA_DIR = process.env.DATA_DIR; +const ORIGINAL_OMNIROUTE_API_KEY = process.env.OMNIROUTE_API_KEY; + +process.env.DATA_DIR = TEST_DATA_DIR; +delete process.env.OMNIROUTE_API_KEY; + +const core = await import("../../src/lib/db/core.ts"); +const { skillRegistry } = await import("../../src/lib/skills/registry.ts"); +const skillsRoute = await import("../../src/app/api/skills/route.ts"); + +const LEAKY_PATH = "/home/testuser/.omniroute/skills/evil/handler.js"; + +function resetStorage() { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); +} + +test.beforeEach(() => { + delete process.env.OMNIROUTE_API_KEY; + resetStorage(); +}); + +test.after(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); + if (ORIGINAL_DATA_DIR === undefined) delete process.env.DATA_DIR; + else process.env.DATA_DIR = ORIGINAL_DATA_DIR; + if (ORIGINAL_OMNIROUTE_API_KEY === undefined) delete process.env.OMNIROUTE_API_KEY; + else process.env.OMNIROUTE_API_KEY = ORIGINAL_OMNIROUTE_API_KEY; +}); + +test("GET /api/skills sanitizes an absolute path out of a 500 error body", async () => { + const original = skillRegistry.loadFromDatabase.bind(skillRegistry); + skillRegistry.loadFromDatabase = async () => { + throw new Error(`EACCES: permission denied, open ${LEAKY_PATH}`); + }; + + try { + const res = await skillsRoute.GET(new Request("http://localhost/api/skills")); + assert.equal(res.status, 500); + + const body = (await res.json()) as { error?: unknown }; + // Shape preserved: still a plain string (the dashboard reads data.error as a string). + assert.equal(typeof body.error, "string"); + const error = body.error as string; + + // The leaked absolute path must be gone; sanitizeErrorMessage replaces it with . + assert.ok( + !error.includes(LEAKY_PATH) && !error.includes("/home/testuser"), + `error must not leak the absolute path, got: ${JSON.stringify(error)}` + ); + assert.ok(error.length > 0, "error should still carry a non-empty message"); + } finally { + skillRegistry.loadFromDatabase = original; + } +});