Harden skill install paths before validation

The install path used registry or SKILL.md names to create temporary and target directories before validation rejected unsafe names. This validates the install name before path construction, keeps the temp root as an explicit cleanup target, and resolves install targets under the selected skills root before copy or force removal.

Constraint: Registry and raw SKILL.md sources are untrusted until validation completes.

Rejected: Rely on validateSkillPath after temp construction | unsafe names can affect filesystem paths before validation runs.

Confidence: high

Scope-risk: narrow

Tested: bun test src/cli/handlers/skills.test.ts src/skills/loadSkillsDir.test.ts src/commands.test.ts

Tested: bun run build

Tested: bun run smoke

Tested: git diff --check

Author: anandh8x

src/cli/handlers/skills.test.ts

@@ -1,6 +1,13 @@
 import assert from 'node:assert/strict'
 import { createHash } from 'node:crypto'
-import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'
+import {
+  existsSync,
+  mkdirSync,
+  mkdtempSync,
+  readFileSync,
+  rmSync,
+  writeFileSync,
+} from 'node:fs'
 import { tmpdir } from 'node:os'
 import { join } from 'node:path'
 import { test } from 'bun:test'
@@ -31,6 +38,20 @@ Use this skill for install tests.
 Document token scopes without storing secret values.
 `
 
+const PATH_TRAVERSAL_SKILL = `---
+name: ../escape
+title: Unsafe Skill
+description: Invalid skill used by install tests.
+version: 0.1.0
+category: test
+author: OpenClaude Tests
+license: MIT
+trust: local
+---
+
+# Unsafe Skill
+`
+
 function skill(
   name: string,
   description: string | undefined,
@@ -231,3 +252,54 @@ test.serial('installs a registry skill by id from a local registry file', async
     assert.equal(installedMetadata.sha256, sha256OfSkillSource(VALID_SKILL))
   })
 })
+
+test.serial('rejects path-like skill names before installing raw markdown', async () => {
+  await withTempDir(async tempDir => {
+    const cwd = join(tempDir, 'project')
+    const sourceDir = join(tempDir, 'source')
+    const sourceFile = join(sourceDir, 'SKILL.md')
+    mkdirSync(sourceDir, { recursive: true })
+    mkdirSync(cwd, { recursive: true })
+    writeFileSync(sourceFile, PATH_TRAVERSAL_SKILL, 'utf8')
+
+    await skillsInstallHandler(sourceFile, { projectDir: cwd })
+
+    assert.equal(process.exitCode, 1)
+    assert.equal(existsSync(join(cwd, '.openclaude', 'skills')), false)
+  })
+})
+
+test.serial('rejects registry names that would escape the install root', async () => {
+  await withTempDir(async tempDir => {
+    const cwd = join(tempDir, 'project')
+    const sourceDir = writeSkillDir(join(tempDir, 'registry-source'))
+    const registryPath = join(tempDir, 'registry.json')
+    mkdirSync(cwd, { recursive: true })
+    writeFileSync(
+      registryPath,
+      JSON.stringify([
+        {
+          id: 'gitlawb/sample-skill',
+          name: '../escape',
+          title: 'Sample Skill',
+          description: 'Sample skill used by install tests.',
+          trust: 'official',
+          version: '0.1.0',
+          license: 'MIT',
+          author: 'OpenClaude Tests',
+          source: join(sourceDir, 'SKILL.md'),
+          sha256: sha256OfSkillSource(VALID_SKILL),
+        },
+      ]),
+      'utf8',
+    )
+
+    await skillsInstallHandler('sample-skill', {
+      projectDir: cwd,
+      registry: registryPath,
+    })
+
+    assert.equal(process.exitCode, 1)
+    assert.equal(existsSync(join(cwd, '.openclaude', 'skills')), false)
+  })
+})

src/cli/handlers/skillsInstall.ts

@@ -1,7 +1,7 @@
 import { createHash } from 'crypto'
 import { cp, mkdir, mkdtemp, readFile, rm, stat, writeFile } from 'fs/promises'
 import { tmpdir } from 'os'
-import { basename, dirname, join, resolve } from 'path'
+import { basename, dirname, isAbsolute, join, relative, resolve } from 'path'
 import { getCwd } from '../../utils/cwd.js'
 import { getClaudeConfigHomeDir } from '../../utils/envUtils.js'
 import { getDisplayPath } from '../../utils/file.js'
@@ -35,6 +35,7 @@ type SkillRegistryEntry = {
 
 const DEFAULT_SKILLS_REGISTRY_URL =
   'https://raw.githubusercontent.com/Gitlawb/openclaude-skills/main/registry.json'
+const VALID_INSTALL_SKILL_NAME = /^[a-z0-9][a-z0-9-]*(?::[a-z0-9][a-z0-9-]*)*$/
 
 function isPlainObject(value: unknown): value is Record<string, unknown> {
   return typeof value === 'object' && value !== null && !Array.isArray(value)
@@ -180,6 +181,34 @@ function skillNameFromSource(source: string): string {
   return leaf.replace(/\.md$/i, '') || 'skill'
 }
 
+function normalizeInstallSkillName(value: string): string {
+  const skillName = value.trim()
+  if (!VALID_INSTALL_SKILL_NAME.test(skillName)) {
+    throw new Error(
+      `Invalid skill name "${value}". Use lowercase letters, numbers, dashes, and optional colon namespaces.`,
+    )
+  }
+  return skillName
+}
+
+function resolveContainedPath(root: string, child: string): string {
+  const resolvedRoot = resolve(root)
+  const resolvedChild = resolve(resolvedRoot, child)
+  const relativePath = relative(resolvedRoot, resolvedChild)
+
+  if (
+    relativePath === '' ||
+    relativePath.startsWith('..') ||
+    isAbsolute(relativePath)
+  ) {
+    throw new Error(
+      `Invalid skill install path "${child}". Skill paths must stay inside ${getDisplayPath(resolvedRoot)}.`,
+    )
+  }
+
+  return resolvedChild
+}
+
 async function prepareSkillFromMarkdown({
   markdown,
   fallbackName,
@@ -188,13 +217,14 @@ async function prepareSkillFromMarkdown({
   markdown: string
   fallbackName: string
   registryEntry?: SkillRegistryEntry
-}): Promise<{ tempDir: string; skillName: string }> {
-  const skillName =
+}): Promise<{ tempRoot: string; tempDir: string; skillName: string }> {
+  const skillName = normalizeInstallSkillName(
     typeof registryEntry?.name === 'string'
       ? registryEntry.name
-      : getSkillNameFromMarkdown(markdown, fallbackName)
+      : getSkillNameFromMarkdown(markdown, fallbackName),
+  )
   const tempRoot = await mkdtemp(join(tmpdir(), 'openclaude-skill-install-'))
-  const tempDir = join(tempRoot, skillName)
+  const tempDir = resolveContainedPath(tempRoot, skillName)
   await mkdir(tempDir, { recursive: true })
   await writeFile(join(tempDir, 'SKILL.md'), markdown, 'utf8')
   if (registryEntry) {
@@ -204,14 +234,15 @@ async function prepareSkillFromMarkdown({
       'utf8',
     )
   }
-  return { tempDir, skillName }
+  return { tempRoot, tempDir, skillName }
 }
 
 async function prepareInstallCandidate(
   spec: string,
   options: InstallOptions,
 ): Promise<{
   tempDir: string
+  tempRoot: string
   skillName: string
   sourceDescription: string
   trust: string
@@ -220,16 +251,17 @@ async function prepareInstallCandidate(
     const sourcePath = resolve(spec)
     const sourceStats = await stat(sourcePath)
     if (sourceStats.isDirectory()) {
-      const skillName = basename(sourcePath)
+      const skillName = normalizeInstallSkillName(basename(sourcePath))
       const tempRoot = await mkdtemp(join(tmpdir(), 'openclaude-skill-install-'))
-      const tempDir = join(tempRoot, skillName)
+      const tempDir = resolveContainedPath(tempRoot, skillName)
       await cp(sourcePath, tempDir, {
         recursive: true,
         errorOnExist: true,
         force: false,
         preserveTimestamps: false,
       })
       return {
+        tempRoot,
         tempDir,
         skillName,
         sourceDescription: getDisplayPath(sourcePath),
@@ -308,7 +340,7 @@ export async function skillsInstallHandler(
     }
 
     const root = installRoot(options)
-    const targetDir = join(root, candidate.skillName)
+    const targetDir = resolveContainedPath(root, candidate.skillName)
     if ((await pathExists(targetDir)) && !options.force) {
       console.error(
         `Skill "${candidate.skillName}" already exists at ${getDisplayPath(targetDir)}. Use --force to overwrite.`,
@@ -339,7 +371,7 @@ export async function skillsInstallHandler(
     process.exitCode = 1
   } finally {
     if (candidate) {
-      await rm(dirname(candidate.tempDir), { recursive: true, force: true })
+      await rm(candidate.tempRoot, { recursive: true, force: true })
     }
   }
 }