fix(opencode): reject path traversal in remote skill discovery

packages/opencode/src/skill/discovery.ts

@@ -1,15 +1,59 @@
 import { LayerNode } from "@opencode-ai/core/effect/layer-node"
 import { httpClient, path } from "@opencode-ai/core/effect/app-node-platform"
-import { NodePath } from "@effect/platform-node"
 import { Effect, Layer, Path, Schema, Context } from "effect"
-import { FetchHttpClient, HttpClient, HttpClientRequest, HttpClientResponse } from "effect/unstable/http"
+import { HttpClient, HttpClientRequest, HttpClientResponse } from "effect/unstable/http"
 import { withTransientReadRetry } from "@/util/effect-http-client"
 import { FSUtil } from "@opencode-ai/core/fs-util"
 import { Global } from "@opencode-ai/core/global"
+import nodePath from "path"
 
 const skillConcurrency = 4
 const fileConcurrency = 8
 
+// ---------------------------------------------------------------------------
+// Validate remote index fields so skill.name / files cannot escape the cache.
+// ---------------------------------------------------------------------------
+
+function isSafeSegment(value: string) {
+  return (
+    value.length > 0 &&
+    value !== "." &&
+    value !== ".." &&
+    !value.includes("/") &&
+    !value.includes("\\") &&
+    !value.includes("\0")
+  )
+}
+
+function isSafeRelativePath(value: string) {
+  const segments = value.split("/")
+  return (
+    value.length > 0 &&
+    !value.includes("\\") &&
+    !value.includes("\0") &&
+    !value.includes("?") &&
+    !value.includes("#") &&
+    !URL.canParse(value) &&
+    !nodePath.posix.isAbsolute(value) &&
+    !nodePath.win32.isAbsolute(value) &&
+    segments.every((segment) => {
+      try {
+        const decoded = decodeURIComponent(segment)
+        return (
+          decoded.length > 0 &&
+          decoded !== "." &&
+          decoded !== ".." &&
+          !decoded.includes("/") &&
+          !decoded.includes("\\") &&
+          !decoded.includes("\0")
+        )
+      } catch {
+        return false
+      }
+    })
+  )
+}
+
 class IndexSkill extends Schema.Class<IndexSkill>("IndexSkill")({
   name: Schema.String,
   files: Schema.Array(Schema.String),
@@ -48,8 +92,8 @@ const layer: Layer.Layer<Service, never, FSUtil.Service | Path.Path | HttpClient
 
     const pull = Effect.fn("Discovery.pull")(function* (url: string) {
       const base = url.endsWith("/") ? url : `${url}/`
-      const index = new URL("index.json", base).href
-      const host = base.slice(0, -1)
+      const source = new URL(base)
+      const index = new URL("index.json", source).href
 
       yield* Effect.logInfo("fetching index", { url: index })
 
@@ -64,19 +108,63 @@ const layer: Layer.Layer<Service, never, FSUtil.Service | Path.Path | HttpClient
 
       if (!data) return []
 
-      const missing = data.skills.filter((skill) => !skill.files.includes("SKILL.md"))
+      const entries = data.skills.flatMap((skill) => {
+        if (!isSafeSegment(skill.name)) {
+          return []
+        }
+        if (!skill.files.includes("SKILL.md")) {
+          return []
+        }
+
+        const root = nodePath.resolve(cache, skill.name)
+        if (!FSUtil.contains(cache, root) || root === cache) {
+          return []
+        }
+
+        const skillUrl = new URL(`${encodeURIComponent(skill.name)}/`, source)
+        const files = skill.files.map((file) => {
+          if (!isSafeRelativePath(file)) return undefined
+          let resource: URL
+          try {
+            resource = new URL(file, skillUrl)
+          } catch {
+            return undefined
+          }
+          if (resource.origin !== source.origin) return undefined
+
+          const destination = nodePath.resolve(root, file)
+          if (!FSUtil.contains(root, destination) || destination === root) return undefined
+          return {
+            url: resource.href,
+            destination,
+            file,
+          }
+        })
+        if (files.some((item) => item === undefined)) {
+          return []
+        }
+        return [
+          {
+            skill,
+            root,
+            files: files as { url: string; destination: string; file: string }[],
+          },
+        ]
+      })
+
+      const missing = data.skills.filter(
+        (skill) => isSafeSegment(skill.name) && !skill.files.includes("SKILL.md"),
+      )
       yield* Effect.forEach(
         missing,
         (skill) => Effect.logWarning("skill entry missing SKILL.md", { url: index, skill: skill.name }),
         { discard: true },
       )
-      const list = data.skills.filter((skill) => skill.files.includes("SKILL.md"))
 
       const dirs = yield* Effect.forEach(
-        list,
-        (skill) =>
+        entries,
+        ({ skill, root, files }) =>
           Effect.gen(function* () {
-            const root = path.join(cache, skill.name)
             const versionFile = path.join(root, ".opencode-version")
             const version = skill.version
             const current =
@@ -85,19 +173,18 @@ const layer: Layer.Layer<Service, never, FSUtil.Service | Path.Path | HttpClient
                 : yield* fs.readFileStringSafe(versionFile).pipe(Effect.catch(() => Effect.succeed(undefined)))
 
             if (version === undefined || current === version) {
-              yield* Effect.forEach(
-                skill.files,
-                (file) => download(new URL(file, `${host}/${skill.name}/`).href, path.join(root, file)),
-                { concurrency: fileConcurrency, discard: true },
-              )
+              yield* Effect.forEach(files, (file) => download(file.url, file.destination), {
+                concurrency: fileConcurrency,
+                discard: true,
+              })
             } else {
               const token = crypto.randomUUID()
               const staging = `${root}.tmp-${token}`
               const backup = `${root}.old-${token}`
               yield* Effect.gen(function* () {
                 const downloaded = yield* Effect.forEach(
-                  skill.files,
-                  (file) => download(new URL(file, `${host}/${skill.name}/`).href, path.join(staging, file)),
+                  files,
+                  (file) => download(file.url, nodePath.resolve(staging, file.file)),
                   { concurrency: fileConcurrency },
                 )
                 if (!downloaded.every(Boolean)) return

packages/opencode/test/skill/discovery.test.ts

@@ -16,6 +16,8 @@ let mutableVersion = "1"
 let mutableContent = "# Old"
 let mutableDownloadCount = 0
 let mutableFiles = ["SKILL.md"]
+let hostileSkills: unknown[] = []
+let hostileDownloads = 0
 
 const fixturePath = path.join(import.meta.dir, "../fixture/skills")
 const cacheDir = path.join(Global.Path.cache, "skills")
@@ -38,6 +40,14 @@ beforeAll(async () => {
       }
       if (url.pathname === "/mutable/mutable/old.md") return new Response("old reference")
 
+      if (url.pathname === "/hostile/index.json") {
+        return Response.json({ skills: hostileSkills })
+      }
+      if (url.pathname.startsWith("/hostile/")) {
+        hostileDownloads++
+        return new Response("should-not-download")
+      }
+
       // route /.well-known/skills/* to the fixture directory
       if (url.pathname.startsWith("/.well-known/skills/")) {
         const filePath = url.pathname.replace("/.well-known/skills/", "")
@@ -183,4 +193,55 @@ describe("Discovery.pull", () => {
       expect(mutableDownloadCount).toBe(3)
     }),
   )
+
+  it.live("rejects skill name traversal without fetching files", () =>
+    Effect.gen(function* () {
+      yield* Effect.promise(() => rm(cacheDir, { recursive: true, force: true }))
+      hostileSkills = [{ name: "../outside", files: ["SKILL.md"] }]
+      hostileDownloads = 0
+      const discovery = yield* Discovery.Service
+      const dirs = yield* discovery.pull(`http://localhost:${server.port}/hostile/`)
+      expect(dirs).toEqual([])
+      expect(hostileDownloads).toBe(0)
+      expect(yield* Effect.promise(() => Bun.file(path.join(Global.Path.cache, "outside", "SKILL.md")).exists())).toBe(
+        false,
+      )
+    }),
+  )
+
+  it.live("rejects file traversal without fetching files", () =>
+    Effect.gen(function* () {
+      yield* Effect.promise(() => rm(cacheDir, { recursive: true, force: true }))
+      hostileSkills = [{ name: "deploy", files: ["SKILL.md", "../outside.md"] }]
+      hostileDownloads = 0
+      const discovery = yield* Discovery.Service
+      const dirs = yield* discovery.pull(`http://localhost:${server.port}/hostile/`)
+      expect(dirs).toEqual([])
+      expect(hostileDownloads).toBe(0)
+    }),
+  )
+
+  it.live("rejects absolute file paths without fetching files", () =>
+    Effect.gen(function* () {
+      yield* Effect.promise(() => rm(cacheDir, { recursive: true, force: true }))
+      hostileSkills = [{ name: "deploy", files: ["SKILL.md", "/tmp/outside.md"] }]
+      hostileDownloads = 0
+      const discovery = yield* Discovery.Service
+      const dirs = yield* discovery.pull(`http://localhost:${server.port}/hostile/`)
+      expect(dirs).toEqual([])
+      expect(hostileDownloads).toBe(0)
+    }),
+  )
+
+  it.live("rejects cross-origin file URLs without fetching files", () =>
+    Effect.gen(function* () {
+      yield* Effect.promise(() => rm(cacheDir, { recursive: true, force: true }))
+      hostileSkills = [{ name: "deploy", files: ["SKILL.md", "https://evil.example.test/outside.md"] }]
+      hostileDownloads = 0
+      const discovery = yield* Discovery.Service
+      const dirs = yield* discovery.pull(`http://localhost:${server.port}/hostile/`)
+      expect(dirs).toEqual([])
+      expect(hostileDownloads).toBe(0)
+    }),
+  )
 })