diff --git a/api/server/routes/skills.js b/api/server/routes/skills.js index 99339d2a29..53a5008bbe 100644 --- a/api/server/routes/skills.js +++ b/api/server/routes/skills.js @@ -346,14 +346,18 @@ router.post( uploadFileHandler, ); +// Wildcard splat (`*relativePath`) captures nested skill paths (e.g. +// `references/guide.md`) whether the client sends an encoded `%2F` or a proxy +// has already decoded it to a literal slash. A single `:relativePath` segment +// 404s in the latter case, which is why nested files failed behind proxies. router.get( - '/:id/files/:relativePath', + '/:id/files/*relativePath', canAccessSkillResource({ requiredPermission: PermissionBits.VIEW }), handlers.downloadFile, ); router.delete( - '/:id/files/:relativePath', + '/:id/files/*relativePath', canAccessSkillResource({ requiredPermission: PermissionBits.EDIT }), handlers.deleteFile, ); diff --git a/api/server/routes/skills.test.js b/api/server/routes/skills.test.js index c48c0ff70b..11157e0ef7 100644 --- a/api/server/routes/skills.test.js +++ b/api/server/routes/skills.test.js @@ -528,7 +528,25 @@ describe('Skill routes', () => { }); }); - describe('GET /api/skills/:id/files/:relativePath', () => { + describe('GET /api/skills/:id/files/*relativePath', () => { + const { upsertSkillFile, updateSkillFileContent } = require('~/models'); + + async function seedNestedFile(skillId, relativePath, content) { + await upsertSkillFile({ + skillId, + relativePath, + file_id: `file-${relativePath}`, + filename: relativePath.split('/').pop(), + filepath: `/tmp/${relativePath}`, + source: 'local', + mimeType: 'text/markdown', + bytes: content.length, + author: testUsers.owner._id, + }); + // Seed cached content so the handler returns it without streaming + await updateSkillFileContent(skillId, relativePath, { content, isBinary: false }); + } + it('returns SKILL.md content from skill body', async () => { const created = await createSkillAsOwner(); const res = await request(app).get(`/api/skills/${created.body._id}/files/SKILL.md`); @@ -539,6 +557,39 @@ describe('Skill routes', () => { expect(res.body.content).toBeDefined(); }); + it('returns a nested file when the path is percent-encoded (%2F)', async () => { + const created = await createSkillAsOwner(); + await seedNestedFile(created.body._id, 'references/working-patterns.md', 'nested body'); + const res = await request(app).get( + `/api/skills/${created.body._id}/files/references%2Fworking-patterns.md`, + ); + expect(res.status).toBe(200); + expect(res.body.relativePath).toBe('references/working-patterns.md'); + expect(res.body.content).toBe('nested body'); + }); + + it('returns a nested file when a proxy decoded %2F to a literal slash', async () => { + const created = await createSkillAsOwner(); + await seedNestedFile(created.body._id, 'references/working-patterns.md', 'nested body'); + const res = await request(app).get( + `/api/skills/${created.body._id}/files/references/working-patterns.md`, + ); + expect(res.status).toBe(200); + expect(res.body.relativePath).toBe('references/working-patterns.md'); + expect(res.body.content).toBe('nested body'); + }); + + it('returns a deeply nested file (multiple subfolders)', async () => { + const created = await createSkillAsOwner(); + await seedNestedFile(created.body._id, 'assets/img/icons/logo.md', 'deep'); + const res = await request(app).get( + `/api/skills/${created.body._id}/files/assets/img/icons/logo.md`, + ); + expect(res.status).toBe(200); + expect(res.body.relativePath).toBe('assets/img/icons/logo.md'); + expect(res.body.content).toBe('deep'); + }); + it('returns 404 for a nonexistent file', async () => { const created = await createSkillAsOwner(); const res = await request(app).get( @@ -546,9 +597,17 @@ describe('Skill routes', () => { ); expect(res.status).toBe(404); }); + + it('returns 404 for a path traversal attempt', async () => { + const created = await createSkillAsOwner(); + const res = await request(app).get( + `/api/skills/${created.body._id}/files/references%2F..%2F..%2Fetc%2Fpasswd`, + ); + expect(res.status).toBe(404); + }); }); - describe('DELETE /api/skills/:id/files/:relativePath', () => { + describe('DELETE /api/skills/:id/files/*relativePath', () => { const { upsertSkillFile } = require('~/models'); it('deletes an existing skill file, bumps skill version, and returns 200', async () => { @@ -584,6 +643,34 @@ describe('Skill routes', () => { expect(afterSkill.body.version).toBe(3); }); + it('deletes a nested file when a proxy decoded %2F to a literal slash', async () => { + const created = await createSkillAsOwner(); + await upsertSkillFile({ + skillId: created.body._id, + relativePath: 'references/notes.md', + file_id: 'file-2', + filename: 'notes.md', + filepath: '/tmp/notes.md', + source: 'local', + mimeType: 'text/markdown', + bytes: 12, + author: testUsers.owner._id, + }); + + const res = await request(app).delete( + `/api/skills/${created.body._id}/files/references/notes.md`, + ); + expect(res.status).toBe(200); + expect(res.body).toEqual({ + skillId: created.body._id, + relativePath: 'references/notes.md', + deleted: true, + }); + + const afterSkill = await request(app).get(`/api/skills/${created.body._id}`); + expect(afterSkill.body.fileCount).toBe(0); + }); + it('returns 404 when the file does not exist', async () => { const created = await createSkillAsOwner(); const res = await request(app).delete( diff --git a/packages/api/src/skills/__tests__/path.test.ts b/packages/api/src/skills/__tests__/path.test.ts new file mode 100644 index 0000000000..69102a83cc --- /dev/null +++ b/packages/api/src/skills/__tests__/path.test.ts @@ -0,0 +1,50 @@ +import { isSafeSkillFilePath, resolveSkillFilePathParam } from '../path'; + +describe('isSafeSkillFilePath', () => { + it('accepts top-level and nested skill file paths', () => { + expect(isSafeSkillFilePath('SKILL.md')).toBe(true); + expect(isSafeSkillFilePath('references/working-patterns.md')).toBe(true); + expect(isSafeSkillFilePath('assets/img/logo.png')).toBe(true); + expect(isSafeSkillFilePath('scripts/run.sh')).toBe(true); + }); + + it('rejects traversal, absolute, and empty-segment paths', () => { + expect(isSafeSkillFilePath('../secrets')).toBe(false); + expect(isSafeSkillFilePath('references/../../etc/passwd')).toBe(false); + expect(isSafeSkillFilePath('/etc/passwd')).toBe(false); + expect(isSafeSkillFilePath('\\windows\\system32')).toBe(false); + expect(isSafeSkillFilePath('references//guide.md')).toBe(false); + expect(isSafeSkillFilePath('./guide.md')).toBe(false); + expect(isSafeSkillFilePath('')).toBe(false); + }); + + it('rejects disallowed characters', () => { + expect(isSafeSkillFilePath('guide file.md')).toBe(false); + expect(isSafeSkillFilePath('guide\0.md')).toBe(false); + expect(isSafeSkillFilePath('weird%20name.md')).toBe(false); + }); +}); + +describe('resolveSkillFilePathParam', () => { + it('joins Express 5 splat segments (proxy decoded a literal slash)', () => { + expect(resolveSkillFilePathParam(['references', 'working-patterns.md'])).toBe( + 'references/working-patterns.md', + ); + expect(resolveSkillFilePathParam(['a', 'b', 'c', 'file.md'])).toBe('a/b/c/file.md'); + }); + + it('handles a single decoded segment (client sent an encoded %2F)', () => { + expect(resolveSkillFilePathParam(['references/working-patterns.md'])).toBe( + 'references/working-patterns.md', + ); + expect(resolveSkillFilePathParam('SKILL.md')).toBe('SKILL.md'); + }); + + it('returns null for empty, missing, or traversal-unsafe params', () => { + expect(resolveSkillFilePathParam(undefined)).toBeNull(); + expect(resolveSkillFilePathParam([])).toBeNull(); + expect(resolveSkillFilePathParam('')).toBeNull(); + expect(resolveSkillFilePathParam(['..', '..', 'etc', 'passwd'])).toBeNull(); + expect(resolveSkillFilePathParam(['../../etc/passwd'])).toBeNull(); + }); +}); diff --git a/packages/api/src/skills/handlers.ts b/packages/api/src/skills/handlers.ts index e2b135c4b0..d4a0f46ed4 100644 --- a/packages/api/src/skills/handlers.ts +++ b/packages/api/src/skills/handlers.ts @@ -32,6 +32,7 @@ import type { import type { Response } from 'express'; import type { Types } from 'mongoose'; import type { ServerRequest, StrategyFunctions } from '~/types'; +import { resolveSkillFilePathParam } from './path'; import { isBinaryBuffer } from './binary'; /** Thin error shape the skill methods throw on validation failure. */ @@ -599,12 +600,12 @@ export function createSkillsHandlers(deps: SkillsHandlersDeps): { async function downloadFileHandler(req: ServerRequest, res: Response) { try { - const { id, relativePath } = req.params as { id: string; relativePath: string }; - let decodedPath: string; - try { - decodedPath = decodeURIComponent(relativePath); - } catch { - return res.status(400).json({ error: 'Invalid file path encoding' }); + const { id } = req.params as { id: string }; + const decodedPath = resolveSkillFilePathParam( + (req.params as { relativePath?: string | string[] }).relativePath, + ); + if (decodedPath == null) { + return res.status(404).json({ error: 'Skill file not found' }); } // SKILL.md is the skill body itself, not a SkillFile document @@ -732,19 +733,19 @@ export function createSkillsHandlers(deps: SkillsHandlersDeps): { } return res.status(200).json({ ...base, isBinary: false, content: text }); } catch (error) { - logger.error('[GET /skills/:id/files/:relativePath] Error', error); + logger.error('[GET /skills/:id/files/*relativePath] Error', error); return res.status(500).json({ error: 'Error downloading skill file' }); } } async function deleteFileHandler(req: ServerRequest, res: Response) { try { - const { id, relativePath } = req.params as { id: string; relativePath: string }; - let decodedPath: string; - try { - decodedPath = decodeURIComponent(relativePath); - } catch { - return res.status(400).json({ error: 'Invalid file path encoding' }); + const { id } = req.params as { id: string }; + const decodedPath = resolveSkillFilePathParam( + (req.params as { relativePath?: string | string[] }).relativePath, + ); + if (decodedPath == null) { + return res.status(404).json({ error: 'Skill file not found' }); } // Look up the file record so we can clean up the storage blob @@ -777,7 +778,7 @@ export function createSkillsHandlers(deps: SkillsHandlersDeps): { }; return res.status(200).json(response); } catch (error) { - logger.error('[DELETE /skills/:id/files/:relativePath] Error', error); + logger.error('[DELETE /skills/:id/files/*relativePath] Error', error); return res.status(500).json({ error: 'Error deleting skill file' }); } } diff --git a/packages/api/src/skills/import.ts b/packages/api/src/skills/import.ts index c93b021593..5e1224a1fb 100644 --- a/packages/api/src/skills/import.ts +++ b/packages/api/src/skills/import.ts @@ -15,6 +15,7 @@ import type { Types } from 'mongoose'; import type { ImportLimits } from './limits'; import { resolveRequestTenantId } from '~/middleware/tenant'; import { DEFAULT_SKILL_IMPORT_LIMITS } from './limits'; +import { isSafeSkillFilePath } from './path'; import { parseSkillMarkdown } from './parse'; const SKILL_MD = 'SKILL.md'; @@ -81,20 +82,6 @@ function sendFrontmatterParseError(res: Response, parseError: string) { }); } -/** Validates a relative path is safe (no traversal, no absolute paths). */ -function isSafePath(p: string): boolean { - if (!p || p.startsWith('/') || p.startsWith('\\')) { - return false; - } - const segments = p.split('/'); - for (const seg of segments) { - if (seg === '..' || seg === '.' || seg === '') { - return false; - } - } - return /^[a-zA-Z0-9._\-/]+$/.test(p); -} - /** Type guard for validation errors thrown by data-schemas. */ function isValidationError(error: unknown): error is Error & { code: string; issues: unknown[] } { return ( @@ -441,7 +428,7 @@ async function handleZip( continue; } - if (!relativePath || !isSafePath(relativePath)) { + if (!relativePath || !isSafeSkillFilePath(relativePath)) { fileResults.push({ path: normalized, status: 'error', error: 'Invalid path' }); continue; } diff --git a/packages/api/src/skills/index.ts b/packages/api/src/skills/index.ts index 5669d6586b..5c0952a73b 100644 --- a/packages/api/src/skills/index.ts +++ b/packages/api/src/skills/index.ts @@ -1,6 +1,7 @@ export * from './binary'; export * from './handlers'; export * from './import'; +export * from './path'; export * from './limits'; export * from './parse'; export * from './skillStates'; diff --git a/packages/api/src/skills/path.ts b/packages/api/src/skills/path.ts new file mode 100644 index 0000000000..fd0cc0c1f5 --- /dev/null +++ b/packages/api/src/skills/path.ts @@ -0,0 +1,38 @@ +/** + * Skill file paths are stored and matched verbatim (e.g. `references/guide.md`). + * A path is safe when it has no absolute prefix, no `.`/`..`/empty segments, and + * uses only the restricted character set skills are imported with. Single source + * of truth shared by import, upload, and the file-serving routes so read, write, + * and delete stay in lockstep. + */ +export function isSafeSkillFilePath(p: string): boolean { + if (!p || p.startsWith('/') || p.startsWith('\\')) { + return false; + } + const segments = p.split('/'); + for (const seg of segments) { + if (seg === '..' || seg === '.' || seg === '') { + return false; + } + } + return /^[a-zA-Z0-9._\-/]+$/.test(p); +} + +/** + * Reconstruct a skill file path from an Express route param. Express 5 splat + * params (`*relativePath`) arrive as an array of already-decoded segments, while + * a single named param arrives as a string. Wildcard routing lets nested files + * resolve whether the client sends an encoded `%2F` or a proxy has already + * decoded it to a literal slash before the request reaches Node. Returns `null` + * for empty or traversal-unsafe paths. + */ +export function resolveSkillFilePathParam(param: string | string[] | undefined): string | null { + if (param == null) { + return null; + } + const joined = Array.isArray(param) ? param.join('/') : param; + if (!isSafeSkillFilePath(joined)) { + return null; + } + return joined; +}