fix(vscode): harden marketplace skill installs

.changeset/calm-marketplace-skills-install.md

@@ -0,0 +1,5 @@
+---
+"kilo-code": patch
+---
+
+Make Marketplace skill installation resilient to missing project directories and overlapping install attempts.

packages/kilo-vscode/src/services/marketplace/installer.ts

@@ -1,6 +1,7 @@
 import * as fs from "fs/promises"
 import * as path from "path"
 import * as os from "os"
+import { randomUUID } from "crypto"
 import * as yaml from "yaml"
 import { exec } from "../../util/process"
 import type {
@@ -100,7 +101,7 @@ export class MarketplaceInstaller {
     await fs.mkdir(dir, { recursive: true })
 
     const filepath = path.join(dir, `${item.id}.md`)
-    if (!path.resolve(filepath).startsWith(path.resolve(dir))) {
+    if (!contains(dir, filepath)) {
       return { success: false, slug: item.id, error: "Invalid agent id" }
     }
 
@@ -132,13 +133,17 @@ export class MarketplaceInstaller {
     scope: "project" | "global",
     workspace?: string,
   ): Promise<RemoveResult> {
+    if (scope === "project" && !workspace) {
+      return { success: false, slug: item.id, error: "No workspace directory for project-scope removal" }
+    }
+
     if (!isSafeId(item.id)) {
       return { success: false, slug: item.id, error: "Invalid agent id" }
     }
 
     const dir = this.paths.agentsDir(scope, workspace)
     const filepath = path.join(dir, `${item.id}.md`)
-    if (!path.resolve(filepath).startsWith(path.resolve(dir))) {
+    if (!contains(dir, filepath)) {
       return { success: false, slug: item.id, error: "Invalid agent id" }
     }
 
@@ -168,6 +173,10 @@ export class MarketplaceInstaller {
     scope: "project" | "global",
     workspace?: string,
   ): Promise<InstallResult> {
+    if (scope === "project" && !workspace) {
+      return { success: false, slug: item.id, error: "No workspace directory for project-scope install" }
+    }
+
     if (!item.content) {
       return { success: false, slug: item.id, error: "Skill has no tarball URL" }
     }
@@ -178,22 +187,18 @@ export class MarketplaceInstaller {
 
     const base = this.paths.skillsDir(scope, workspace)
     const dir = path.join(base, item.id)
-    if (!path.resolve(dir).startsWith(path.resolve(base))) {
+    if (!contains(base, dir)) {
       return { success: false, slug: item.id, error: "Invalid skill id" }
     }
 
-    try {
-      await fs.access(dir)
+    if (await exists(dir)) {
       return { success: false, slug: item.id, error: "Skill already installed. Uninstall it before installing again." }
-    } catch (err) {
-      if ((err as NodeJS.ErrnoException).code !== "ENOENT") throw err
     }
 
-    const stamp = Date.now()
-    const tarball = path.join(os.tmpdir(), `kilo-skill-${item.id}-${stamp}.tar.gz`)
     // Stage under `base` (not os.tmpdir()) so fs.rename() never crosses filesystems (EXDEV).
     await fs.mkdir(base, { recursive: true })
-    const staging = path.join(base, `.staging-${item.id}-${stamp}`)
+    const staging = await fs.mkdtemp(path.join(base, `.staging-${item.id}-`))
+    const tarball = path.join(os.tmpdir(), `kilo-skill-${item.id}-${randomUUID()}.tar.gz`)
 
     try {
       const response = await fetch(item.content)
@@ -203,14 +208,11 @@ export class MarketplaceInstaller {
 
       const buffer = Buffer.from(await response.arrayBuffer())
       await fs.writeFile(tarball, buffer)
-
-      await fs.mkdir(staging, { recursive: true })
       await exec("tar", ["-xzf", tarball, "--strip-components=1", "-C", staging])
 
       const escaped = await findEscapedPaths(staging)
       if (escaped.length > 0) {
         console.warn(`Skill archive ${item.id} contains escaped paths:`, escaped)
-        await fs.rm(staging, { recursive: true })
         return { success: false, slug: item.id, error: "Skill archive contains unsafe paths" }
       }
 
@@ -218,7 +220,6 @@ export class MarketplaceInstaller {
         await fs.access(path.join(staging, "SKILL.md"))
       } catch {
         console.warn(`Extracted skill ${item.id} missing SKILL.md, rolling back`)
-        await fs.rm(staging, { recursive: true })
         return { success: false, slug: item.id, error: "Extracted archive missing SKILL.md" }
       }
 
@@ -226,19 +227,24 @@ export class MarketplaceInstaller {
 
       return { success: true, slug: item.id, filePath: path.join(dir, "SKILL.md"), line: 1 }
     } catch (err) {
-      console.warn(`Failed to install skill ${item.id}:`, err)
-      try {
-        await fs.rm(staging, { recursive: true })
-      } catch {
-        console.warn(`Failed to clean up staging directory ${staging}`)
+      if (await exists(dir)) {
+        return {
+          success: false,
+          slug: item.id,
+          error: "Skill already installed. Uninstall it before installing again.",
+        }
       }
+      console.warn(`Failed to install skill ${item.id}:`, err)
       return { success: false, slug: item.id, error: String(err) }
     } finally {
-      try {
-        await fs.unlink(tarball)
-      } catch {
-        console.warn(`Failed to clean up temp file ${tarball}`)
-      }
+      await Promise.all([
+        fs.rm(staging, { recursive: true, force: true }).catch((err) => {
+          console.warn(`Failed to clean up staging directory ${staging}:`, err)
+        }),
+        fs.rm(tarball, { force: true }).catch((err) => {
+          console.warn(`Failed to clean up temp file ${tarball}:`, err)
+        }),
+      ])
     }
   }
 
@@ -251,6 +257,10 @@ export class MarketplaceInstaller {
   }
 
   async removeMcp(item: McpMarketplaceItem, scope: "project" | "global", workspace?: string): Promise<RemoveResult> {
+    if (scope === "project" && !workspace) {
+      return { success: false, slug: item.id, error: "No workspace directory for project-scope removal" }
+    }
+
     const config = await this.readConfig(scope, workspace)
     if (!config.mcp?.[item.id]) {
       return { success: true, slug: item.id }
@@ -266,12 +276,16 @@ export class MarketplaceInstaller {
     scope: "project" | "global",
     workspace?: string,
   ): Promise<RemoveResult> {
+    if (scope === "project" && !workspace) {
+      return { success: false, slug: item.id, error: "No workspace directory for project-scope removal" }
+    }
+
     if (!isSafeId(item.id)) {
       return { success: false, slug: item.id, error: "Invalid skill id" }
     }
     const base = this.paths.skillsDir(scope, workspace)
     const dir = path.join(base, item.id)
-    if (!path.resolve(dir).startsWith(path.resolve(base))) {
+    if (!contains(base, dir)) {
       return { success: false, slug: item.id, error: "Invalid skill id" }
     }
     try {
@@ -316,6 +330,20 @@ export class MarketplaceInstaller {
 
 // ── Helpers ─────────────────────────────────────────────────────────
 
+async function exists(filepath: string): Promise<boolean> {
+  try {
+    await fs.access(filepath)
+    return true
+  } catch (err) {
+    if ((err as NodeJS.ErrnoException).code === "ENOENT") return false
+    throw err
+  }
+}
+
+function contains(dir: string, filepath: string): boolean {
+  return path.resolve(filepath).startsWith(path.resolve(dir) + path.sep)
+}
+
 /**
  * Normalize a marketplace MCP entry from the old Kilocode format to the CLI's expected format.
  *
@@ -359,7 +387,8 @@ function normalizeMcpEntry(raw: Record<string, unknown>): Record<string, unknown
 }
 
 function isSafeId(id: string): boolean {
-  if (!id || id.includes("..") || id.includes("/") || id.includes("\\")) return false
+  if (!id || id === "." || id.includes("..") || id.includes("/") || id.includes("\\") || id.endsWith(".")) return false
+  if (/^(con|prn|aux|nul|com[1-9]|lpt[1-9])(\.|$)/i.test(id)) return false
   return /^[\w\-@.]+$/.test(id)
 }
 

packages/kilo-vscode/tests/unit/marketplace-installer.test.ts

@@ -4,6 +4,7 @@ import * as os from "os"
 import * as path from "path"
 import { MarketplaceInstaller } from "../../src/services/marketplace/installer"
 import { MarketplacePaths } from "../../src/services/marketplace/paths"
+import { exec } from "../../src/util/process"
 
 const tmpDir = path.join(os.tmpdir(), `kilo-test-${Date.now()}`)
 
@@ -17,11 +18,36 @@ class TestPaths extends MarketplacePaths {
   }
 }
 
-describe("MarketplaceInstaller MCP format normalization", () => {
-  afterEach(async () => {
-    await fs.rm(tmpDir, { recursive: true }).catch(() => {})
-  })
+function skill(content: string, id = "test-skill") {
+  return {
+    type: "skill" as const,
+    id,
+    name: "Test Skill",
+    description: "test",
+    category: "test",
+    githubUrl: "https://example.com",
+    content,
+    displayName: "Test Skill",
+    displayCategory: "Test",
+  }
+}
 
+async function archive(): Promise<Buffer> {
+  const root = path.join(tmpDir, "archive")
+  const source = path.join(root, "source")
+  const dir = path.join(source, "skill")
+  const tarball = path.join(root, "skill.tar.gz")
+  await fs.mkdir(dir, { recursive: true })
+  await fs.writeFile(path.join(dir, "SKILL.md"), "# Test Skill\n")
+  await exec("tar", ["-czf", tarball, "-C", source, "skill"])
+  return fs.readFile(tarball)
+}
+
+afterEach(async () => {
+  await fs.rm(tmpDir, { recursive: true, force: true })
+})
+
+describe("MarketplaceInstaller MCP format normalization", () => {
   it("converts local command+args+env format to CLI format", async () => {
     const installer = new MarketplaceInstaller(new TestPaths())
     const item = {
@@ -94,3 +120,129 @@ describe("MarketplaceInstaller MCP format normalization", () => {
     expect(mcp).toEqual({ type: "local", command: ["npx", "-y", "someserver"], environment: { KEY: "val" } })
   })
 })
+
+describe("MarketplaceInstaller skills", () => {
+  it("rejects project installs without a workspace directory", async () => {
+    const installer = new MarketplaceInstaller(new TestPaths())
+    const result = await installer.installSkill(skill("https://example.com/skill.tar.gz"), "project")
+
+    expect(result).toEqual({
+      success: false,
+      slug: "test-skill",
+      error: "No workspace directory for project-scope install",
+    })
+  })
+
+  it("rejects project removals without a workspace directory", async () => {
+    const installer = new MarketplaceInstaller(new TestPaths())
+    const result = await installer.removeSkill(skill("https://example.com/skill.tar.gz"), "project")
+
+    expect(result).toEqual({
+      success: false,
+      slug: "test-skill",
+      error: "No workspace directory for project-scope removal",
+    })
+  })
+
+  it("rejects project MCP and agent removals without a workspace directory", async () => {
+    const installer = new MarketplaceInstaller(new TestPaths())
+    const results = await Promise.all([
+      installer.remove(
+        {
+          type: "mcp",
+          id: "test-mcp",
+          name: "Test MCP",
+          description: "test",
+          url: "https://example.com",
+          content: "{}",
+        },
+        "project",
+      ),
+      installer.remove(
+        {
+          type: "agent",
+          id: "test-agent",
+          name: "Test Agent",
+          description: "test",
+          content: { mode: "all", description: "test", prompt: "test" },
+        },
+        "project",
+      ),
+    ])
+
+    expect(results).toEqual([
+      { success: false, slug: "test-mcp", error: "No workspace directory for project-scope removal" },
+      { success: false, slug: "test-agent", error: "No workspace directory for project-scope removal" },
+    ])
+  })
+
+  it("rejects skill ids that are unsafe on supported filesystems", async () => {
+    const paths = new TestPaths()
+    const dir = path.join(paths.skillsDir("project", tmpDir), "installed")
+    await fs.mkdir(dir, { recursive: true })
+    await fs.writeFile(path.join(dir, "SKILL.md"), "# Installed\n")
+    const installer = new MarketplaceInstaller(paths)
+
+    for (const id of [".", "installed.", "CON", "nul.txt"]) {
+      const result = await installer.removeSkill(skill("https://example.com/skill.tar.gz", id), "project", tmpDir)
+      expect(result).toEqual({ success: false, slug: id, error: "Invalid skill id" })
+    }
+
+    expect(await fs.readFile(path.join(dir, "SKILL.md"), "utf-8")).toBe("# Installed\n")
+  })
+
+  it("installs an extracted project skill without leaving staging directories", async () => {
+    const buffer = await archive()
+    const url = `data:application/gzip;base64,${buffer.toString("base64")}`
+    const paths = new TestPaths()
+    const installer = new MarketplaceInstaller(paths)
+    const result = await installer.installSkill(skill(url), "project", tmpDir)
+
+    expect(result.success).toBe(true)
+    expect(await fs.readFile(path.join(paths.skillsDir("project", tmpDir), "test-skill", "SKILL.md"), "utf-8")).toBe(
+      "# Test Skill\n",
+    )
+    expect(
+      (await fs.readdir(paths.skillsDir("project", tmpDir))).filter((name) => name.startsWith(".staging-")),
+    ).toEqual([])
+  })
+
+  it("handles concurrent installs without sharing temporary paths", async () => {
+    const buffer = await archive()
+    const original = globalThis.fetch
+    const now = Date.now
+    const paths = new TestPaths()
+    const installer = new MarketplaceInstaller(paths)
+    const item = skill("https://example.com/skill.tar.gz")
+    const gate = Promise.withResolvers<void>()
+    let count = 0
+    Date.now = () => 1
+    globalThis.fetch = async () => {
+      count += 1
+      if (count === 2) gate.resolve()
+      await gate.promise
+      return new Response(buffer)
+    }
+
+    try {
+      const results = await Promise.all([
+        installer.installSkill(item, "project", tmpDir),
+        installer.installSkill(item, "project", tmpDir),
+      ])
+      expect(count).toBe(2)
+      expect(results.filter((result) => result.success)).toHaveLength(1)
+      expect(results.find((result) => !result.success)?.error).toBe(
+        "Skill already installed. Uninstall it before installing again.",
+      )
+      expect(await fs.readFile(path.join(paths.skillsDir("project", tmpDir), "test-skill", "SKILL.md"), "utf-8")).toBe(
+        "# Test Skill\n",
+      )
+      expect(
+        (await fs.readdir(paths.skillsDir("project", tmpDir))).filter((name) => name.startsWith(".staging-")),
+      ).toEqual([])
+    } finally {
+      globalThis.fetch = original
+      Date.now = now
+    }
+  })
+})