Skip to content

fix(api/skills): sanitize error messages before returning them to clients - #9088

Merged
diegosouzapw merged 2 commits into
diegosouzapw:release/v3.8.50from
pacocartones:fix/skills-error-sanitization
Aug 6, 2026
Merged

diegosouzapw merged 2 commits into
diegosouzapw:release/v3.8.50from
pacocartones:fix/skills-error-sanitization

Conversation

@pacocartones

Copy link
Copy Markdown
Contributor

What & why

The src/app/api/skills/** routes returned the raw error message to the client on a 500:

} catch (err: unknown) {
  const error = err instanceof Error ? err.message : String(err);
  return NextResponse.json({ error }, { status: 500 });
}

skillRegistry reads/writes the skills directory, so a filesystem failure (EACCES/ENOSPC) surfaces an absolute path verbatim to the caller, e.g. EACCES: permission denied, open '/home/<user>/.omniroute/skills/<name>/handler.js'. That's the stack/path leak Hard Rule #12 — and the repo's own sanitizeErrorMessage — already guard against (126 of 175 src/app/api/**/route.ts files sanitize; these skills routes were among the ~49 that didn't).

Change

Route the caught message through sanitizeErrorMessage in the 10 catch blocks across 8 skills routes:

const error = sanitizeErrorMessage(err instanceof Error ? err.message : String(err));

sanitizeErrorMessage is linear-time (CodeQL js/polynomial-redos-clean) and strips stack tails + absolute source paths and redacts sensitive tokens, leaving ordinary messages intact — so the executions route's if (error.includes("disabled")) 503 branch still works.

Scope decisions (deliberate)

  • Shape kept as { error: string }, not migrated to buildErrorBody. The omni-skills dashboard reads data.error as a string (OmniMarketplaceTab.tsx:53/83/100/129, OmniSkillsPageClient.tsx:153: (data as { error?: string }).error). Returning { error: { message } } would render [object Object] in the UI. This PR fixes the leak without changing the contract.
  • collect/detect/route.ts left untouched — it already uses buildErrorBody.

Files: skills/route.ts, [id]/route.ts (×2), executions/route.ts (×2), install/route.ts, marketplace/route.ts, marketplace/install/route.ts, skillssh/route.ts, skillssh/install/route.ts.

Tests

Adds tests/unit/skills-routes-error-sanitization.test.ts: monkey-patches skillRegistry.loadFromDatabase to throw an error carrying an absolute path, calls GET /api/skills, and asserts the 500 body is still a string and no longer contains the path.

✔ tests/unit/skills-routes-error-sanitization.test.ts — 1 pass

No response-shape change; no public behavior change beyond redacted error text.

…ents

The /api/skills/** routes returned `{ error: err.message }` verbatim on a
500. Because skillRegistry performs filesystem work, an EACCES/ENOSPC surfaces
an absolute path (e.g. `open '/home/<user>/.omniroute/skills/<name>/handler.js'`)
straight to the caller — the exact stack/path leak Hard Rule diegosouzapw#12 and the repo's
own sanitizeErrorMessage guard against (126 of 175 API routes already sanitize).

Route the caught message through sanitizeErrorMessage in the 10 catch blocks
across the 8 skills routes. The response shape is deliberately left as
`{ error: string }`: the omni-skills dashboard reads `data.error` as a string
(OmniMarketplaceTab.tsx, OmniSkillsPageClient.tsx), so switching to
buildErrorBody's `{ error: { message } }` would break the UI. collect/detect
already uses buildErrorBody and is left untouched.

Adds tests/unit/skills-routes-error-sanitization.test.ts: forces
skillRegistry.loadFromDatabase to throw an error carrying an absolute path and
asserts GET /api/skills returns a 500 whose string body no longer leaks it.
@diegosouzapw
diegosouzapw merged commit bca61af into diegosouzapw:release/v3.8.50 Aug 6, 2026
3 checks passed
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…ents (diegosouzapw#9088)

Validated in local merge-train T5 (base49+contributors+pacocartones)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants