🪆 fix: Serve Nested Skill Files Through a Wildcard Route (#14191)

Nested skill files (references/, scripts/, assets/) returned 404 when
opened, while top-level SKILL.md worked. The file routes matched a single
`:relativePath` segment, so a nested path only resolves when the encoded
`%2F` reaches Node intact. Reverse proxies (nginx, Traefik, k8s ingress,
ALB, Cloudflare) commonly normalize `%2F` to a literal slash before
forwarding, which turns the path into multiple segments the single-segment
route can no longer match.

Switch the GET/DELETE file routes to an Express 5 splat (`*relativePath`)
and reconstruct the path from the decoded segments, so nested files resolve
whether the client sends `%2F` or a proxy decoded it to a literal slash.

Security: add a shared `isSafeSkillFilePath` validator (extracted from the
import validator, single source of truth) and reject traversal/absolute/
empty-segment paths at the route layer. Lookups remain exact DB matches, so
the user-supplied path never touches the filesystem.

Also drops a latent double-decode (Express already decodes route params).

Closes #14190

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,
 );

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(

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();
+  });
+});

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' });
     }
   }

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;
     }

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';

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;
+}