fix: skills CRUD broken on Windows due to hardcoded path separator (#492)

safeSkillDir() used a hardcoded `/` in the startsWith path traversal
check. On Windows, path.resolve() returns backslash paths, so the check
always failed — blocking getSkill, createSkill, updateSkill, deleteSkill.

Replace `${skillsDir}/` with `${skillsDir}${sep}` using path.sep from
node:path, which returns `\` on Windows and `/` on POSIX.

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Nikhil
2026-03-12 14:10:20 -07:00
committed by GitHub
parent b94e9c411d
commit 32dd42cc6b
2 changed files with 29 additions and 2 deletions

View File

@@ -1,5 +1,5 @@
import { mkdir, readFile, rm, stat, writeFile } from 'node:fs/promises'
import { join, resolve } from 'node:path'
import { join, resolve, sep } from 'node:path'
import matter from 'gray-matter'
import { getSkillsDir } from '../lib/browseros-dir'
import { logger } from '../lib/logger'
@@ -23,7 +23,7 @@ export function slugify(name: string): string {
function safeSkillDir(id: string): string {
const skillsDir = getSkillsDir()
const resolved = resolve(skillsDir, id)
if (!resolved.startsWith(`${skillsDir}/`)) {
if (!resolved.startsWith(`${skillsDir}${sep}`)) {
throw new Error('Invalid skill id')
}
return resolved

View File

@@ -0,0 +1,27 @@
import { describe, it } from 'bun:test'
import assert from 'node:assert'
import { sep } from 'node:path'
import { createSkillsRoutes } from '../../src/api/routes/skills'
describe('skills routes', () => {
const app = createSkillsRoutes()
it('GET /:id returns 404 for non-existent skill (not 500 from path check)', async () => {
const res = await app.request('/valid-skill-id')
assert.strictEqual(res.status, 404)
const body = await res.json()
assert.strictEqual(body.error, 'Skill not found')
})
it('GET /:id rejects path traversal attempts', async () => {
const res = await app.request('/../../../etc/passwd')
assert.notStrictEqual(res.status, 200)
})
})
describe('safeSkillDir uses platform separator', () => {
it(`path.sep is "${sep}" on this platform`, () => {
assert.ok(sep === '/' || sep === '\\')
})
})