From 09f4e8e1429a8b39e1412bf8e61aeb9f2070dd18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E4=BC=A0=E5=BA=B7Kk?= <35034498+1837620622@users.noreply.github.com> Date: Sat, 11 Jul 2026 11:45:24 +0800 Subject: [PATCH 1/2] fix(opencode): reject path traversal in remote skill discovery Validate skill.name and files from remote skill indexes before writing into the local cache. Align with packages/core skill discovery so a hostile index cannot escape the skills cache or fetch cross-origin resources. --- packages/opencode/src/skill/discovery.ts | 119 +++++++++++++++--- .../opencode/test/skill/discovery.test.ts | 61 +++++++++ 2 files changed, 164 insertions(+), 16 deletions(-) diff --git a/packages/opencode/src/skill/discovery.ts b/packages/opencode/src/skill/discovery.ts index b56a67610f82..50c277475969 100644 --- a/packages/opencode/src/skill/discovery.ts +++ b/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 +// --------------------------------------------------------------------------- +// 远程 index 字段校验：防止 skill.name / files 路径穿越写出缓存目录 +// --------------------------------------------------------------------------- + +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")({ name: Schema.String, files: Schema.Array(Schema.String), @@ -48,8 +92,8 @@ const layer: Layer.Layer !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 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 diff --git a/packages/opencode/test/skill/discovery.test.ts b/packages/opencode/test/skill/discovery.test.ts index bdb60dfddc8d..24eb92ee3a9c 100644 --- a/packages/opencode/test/skill/discovery.test.ts +++ b/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) + }), + ) }) From fbe97a4021c3961552c66bb20eccd16a609cc652 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E4=BC=A0=E5=BA=B7Kk?= <35034498+1837620622@users.noreply.github.com> Date: Sat, 11 Jul 2026 11:51:30 +0800 Subject: [PATCH 2/2] fix(opencode): use English comment for skill path safety helpers Keep the remote skill discovery path-traversal guards; only switch the section comment to English for upstream consistency. --- packages/opencode/src/skill/discovery.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opencode/src/skill/discovery.ts b/packages/opencode/src/skill/discovery.ts index 50c277475969..45ef8fb1b60c 100644 --- a/packages/opencode/src/skill/discovery.ts +++ b/packages/opencode/src/skill/discovery.ts @@ -11,7 +11,7 @@ const skillConcurrency = 4 const fileConcurrency = 8 // --------------------------------------------------------------------------- -// 远程 index 字段校验：防止 skill.name / files 路径穿越写出缓存目录 +// Validate remote index fields so skill.name / files cannot escape the cache. // --------------------------------------------------------------------------- function isSafeSegment(value: string) {