From cc028d36e71de374e962615f127675862eea0ea6 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 30 May 2026 11:50:30 -0700 Subject: [PATCH] fix: enforce skill discovery depth for dotdot names (#3725) --- src/main/skills/discovery.test.ts | 15 ++++ src/main/skills/discovery.ts | 92 ++++------------------ src/main/skills/skill-discovery-sources.ts | 77 ++++++++++++++++++ 3 files changed, 106 insertions(+), 78 deletions(-) create mode 100644 src/main/skills/skill-discovery-sources.ts diff --git a/src/main/skills/discovery.test.ts b/src/main/skills/discovery.test.ts index 00fe9381314..2eeba6ddac4 100644 --- a/src/main/skills/discovery.test.ts +++ b/src/main/skills/discovery.test.ts @@ -94,4 +94,19 @@ describe('skill discovery', () => { expect(result.skills).toEqual([]) }) + + it('enforces depth limits for valid child directories whose names start with dot-dot', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-skills-')) + const home = join(root, 'home') + const deepSkill = join(home, '.agents', 'skills', '..deep', 'a', 'b', 'c', 'd', 'too-deep') + await mkdir(deepSkill, { recursive: true }) + await writeFile(join(deepSkill, 'SKILL.md'), '# Too Deep\n\nShould not be discovered.') + + const result = await discoverSkills({ + homeDir: home, + cwd: join(root, 'missing-cwd') + }) + + expect(result.skills.map((skill) => skill.name)).not.toContain('Too Deep') + }) }) diff --git a/src/main/skills/discovery.ts b/src/main/skills/discovery.ts index 9ea8c02d8c1..3b19444cb01 100644 --- a/src/main/skills/discovery.ts +++ b/src/main/skills/discovery.ts @@ -1,19 +1,21 @@ -import { createHash } from 'node:crypto' import type { Dirent } from 'node:fs' import { open, readdir, realpath, stat } from 'node:fs/promises' -import { homedir } from 'node:os' -import { basename, dirname, join, relative, sep } from 'node:path' -import type { Repo } from '../../shared/types' +import { basename, dirname, isAbsolute, join, relative, sep } from 'node:path' import { summarizeSkillMarkdown } from '../../shared/skill-metadata' +import type { Repo } from '../../shared/types' import type { DiscoveredSkill, SkillDiscoveryResult, SkillDiscoverySource, - SkillProvider, SkillSourceKind } from '../../shared/skills' +import { + buildSkillDiscoverySources, + stablePathId, + type SkillScanRoot +} from './skill-discovery-sources' -type SkillScanRoot = Omit +export { buildSkillDiscoverySources } from './skill-discovery-sources' const SKILL_FILE_NAME = 'SKILL.md' const MAX_MARKDOWN_BYTES = 256 * 1024 @@ -28,10 +30,6 @@ async function pathExists(pathValue: string): Promise { } } -function stableId(pathValue: string): string { - return createHash('sha1').update(pathValue).digest('hex').slice(0, 16) -} - function compareSkills(a: DiscoveredSkill, b: DiscoveredSkill): number { return ( a.name.localeCompare(b.name, undefined, { sensitivity: 'base' }) || @@ -42,9 +40,13 @@ function compareSkills(a: DiscoveredSkill, b: DiscoveredSkill): number { function isWithinDepth(rootPath: string, childPath: string, maxDepth: number): boolean { const rel = relative(rootPath, childPath) - if (!rel || rel.startsWith('..')) { + if (!rel) { return true } + // Why: `..cache` is a valid child name; only a real parent traversal escapes. + if (rel === '..' || rel.startsWith(`..${sep}`) || isAbsolute(rel)) { + return false + } return rel.split(sep).length <= maxDepth } @@ -210,7 +212,7 @@ async function scanRoot(root: SkillScanRoot): Promise { const summary = await readSkillSummary(skillFilePath) const sourceKind = sourceKindForSkill(root, skillFilePath) return { - id: stableId(skillFilePath), + id: stablePathId(skillFilePath), name: summary.name ?? basename(directoryPath), description: summary.description, providers: root.providers, @@ -228,72 +230,6 @@ async function scanRoot(root: SkillScanRoot): Promise { return skills } -function source( - id: string, - label: string, - path: string, - sourceKind: SkillSourceKind, - providers: SkillProvider[] -): SkillScanRoot { - return { id, label, path, sourceKind, providers } -} - -export function buildSkillDiscoverySources( - args: { - homeDir?: string - cwd?: string - repos?: Repo[] - } = {} -): SkillScanRoot[] { - const home = args.homeDir ?? homedir() - const cwd = args.cwd ?? process.cwd() - const roots: SkillScanRoot[] = [ - source('home-codex', 'Codex home', join(home, '.codex', 'skills'), 'home', ['codex']), - source('home-agents', 'Agent skills home', join(home, '.agents', 'skills'), 'home', [ - 'agent-skills' - ]), - source('home-claude', 'Claude home', join(home, '.claude', 'skills'), 'home', ['claude']), - source( - 'codex-plugin-cache', - 'Codex plugin cache', - join(home, '.codex', 'plugins', 'cache'), - 'plugin', - ['codex', 'agent-skills'] - ) - ] - - const projectPaths = new Set() - for (const repo of args.repos ?? []) { - if (repo.connectionId) { - continue - } - projectPaths.add(repo.path) - } - projectPaths.add(cwd) - - for (const repoPath of projectPaths) { - const label = `Repo ${basename(repoPath)}` - roots.push( - source( - `repo-agents-${stableId(repoPath)}`, - `${label} .agents`, - join(repoPath, '.agents', 'skills'), - 'repo', - ['agent-skills'] - ), - source( - `repo-claude-${stableId(repoPath)}`, - `${label} .claude`, - join(repoPath, '.claude', 'skills'), - 'repo', - ['claude'] - ) - ) - } - - return roots -} - export async function discoverSkills(args: { repos?: Repo[] homeDir?: string diff --git a/src/main/skills/skill-discovery-sources.ts b/src/main/skills/skill-discovery-sources.ts new file mode 100644 index 00000000000..c1bb1072b2e --- /dev/null +++ b/src/main/skills/skill-discovery-sources.ts @@ -0,0 +1,77 @@ +import { createHash } from 'node:crypto' +import { homedir } from 'node:os' +import { basename, join } from 'node:path' +import type { SkillDiscoverySource, SkillProvider, SkillSourceKind } from '../../shared/skills' +import type { Repo } from '../../shared/types' + +export type SkillScanRoot = Omit + +export function stablePathId(pathValue: string): string { + return createHash('sha1').update(pathValue).digest('hex').slice(0, 16) +} + +function source( + id: string, + label: string, + path: string, + sourceKind: SkillSourceKind, + providers: SkillProvider[] +): SkillScanRoot { + return { id, label, path, sourceKind, providers } +} + +export function buildSkillDiscoverySources( + args: { + homeDir?: string + cwd?: string + repos?: Repo[] + } = {} +): SkillScanRoot[] { + const home = args.homeDir ?? homedir() + const cwd = args.cwd ?? process.cwd() + const roots: SkillScanRoot[] = [ + source('home-codex', 'Codex home', join(home, '.codex', 'skills'), 'home', ['codex']), + source('home-agents', 'Agent skills home', join(home, '.agents', 'skills'), 'home', [ + 'agent-skills' + ]), + source('home-claude', 'Claude home', join(home, '.claude', 'skills'), 'home', ['claude']), + source( + 'codex-plugin-cache', + 'Codex plugin cache', + join(home, '.codex', 'plugins', 'cache'), + 'plugin', + ['codex', 'agent-skills'] + ) + ] + + const projectPaths = new Set() + for (const repo of args.repos ?? []) { + if (repo.connectionId) { + continue + } + projectPaths.add(repo.path) + } + projectPaths.add(cwd) + + for (const repoPath of projectPaths) { + const label = `Repo ${basename(repoPath)}` + roots.push( + source( + `repo-agents-${stablePathId(repoPath)}`, + `${label} .agents`, + join(repoPath, '.agents', 'skills'), + 'repo', + ['agent-skills'] + ), + source( + `repo-claude-${stablePathId(repoPath)}`, + `${label} .claude`, + join(repoPath, '.claude', 'skills'), + 'repo', + ['claude'] + ) + ) + } + + return roots +}