From da328dfd8e9221b432f67f0b54828279f6a0fd5b Mon Sep 17 00:00:00 2001 From: Bruno Agatao Date: Wed, 29 Jul 2026 16:26:45 +0200 Subject: [PATCH] fix(cli): validate and origin-pin remote skill downloads --- packages/opencode/src/skill/discovery.ts | 105 ++++++++++++------ .../opencode/test/skill/discovery.test.ts | 13 +++ 2 files changed, 87 insertions(+), 31 deletions(-) diff --git a/packages/opencode/src/skill/discovery.ts b/packages/opencode/src/skill/discovery.ts index 24e78c14f8..f4e0c50d22 100644 --- a/packages/opencode/src/skill/discovery.ts +++ b/packages/opencode/src/skill/discovery.ts @@ -1,3 +1,4 @@ +import { posix, win32 } from "node:path" // kilocode_change - pure segment/path validation helpers import { LayerNode } from "@opencode-ai/core/effect/layer-node" import { httpClient, path } from "@opencode-ai/core/effect/layer-node-platform" import { NodePath } from "@effect/platform-node" @@ -10,6 +11,45 @@ import { Global } from "@opencode-ai/core/global" const skillConcurrency = 4 const fileConcurrency = 8 +// kilocode_change start - segment/relative-path validation mirrors core v2 SkillDiscovery so a remote +// index cannot smuggle traversal, absolute paths, URLs, or null bytes into a cache write target. +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) && + !posix.isAbsolute(value) && + !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 + } + }) + ) +} + +// kilocode_change end + class IndexSkill extends Schema.Class("IndexSkill")({ name: Schema.String, files: Schema.Array(Schema.String), @@ -47,8 +87,8 @@ export const layer: Layer.Layer !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")) - - // kilocode_change start - remote index.json controls skill.name/file, so a crafted `../` could escape the - // cache and plant a SKILL.md in a trusted dir (e.g. ~/.agents/skills). Drop any skill whose paths escape it. - const rooted = (target: string) => { - const rel = path.relative(cache, target) + // kilocode_change start - the remote index controls skill.name and file, so validate every segment, + // pin file downloads to the index origin, and confine writes to the cache (mirrors core v2 SkillDiscovery) + const contained = (parent: string, child: string) => { + const rel = path.relative(parent, child) return rel !== "" && !rel.startsWith("..") && !path.isAbsolute(rel) } - const safe: typeof list = [] - for (const skill of list) { + const plan = (skill: IndexSkill) => { + if (!skill.files.includes("SKILL.md")) return "skill entry missing SKILL.md" + if (!isSafeSegment(skill.name)) return "skipping skill with unsafe name" const root = path.join(cache, skill.name) - if (rooted(root) && skill.files.every((file) => rooted(path.join(root, file)))) safe.push(skill) - else yield* Effect.logWarning("skipping skill with unsafe path", { url: index, skill: skill.name }) + if (!contained(cache, root)) return "skipping skill with unsafe name" + const skillUrl = new URL(`${encodeURIComponent(skill.name)}/`, source) + const files: { url: string; dest: string }[] = [] + for (const file of skill.files) { + if (!isSafeRelativePath(file)) return "skipping skill with unsafe file path" + const resource = URL.parse(file, skillUrl) ?? undefined + if (!resource || resource.origin !== source.origin) return "skipping skill with cross-origin file" + const dest = path.join(root, file) + if (!contained(root, dest)) return "skipping skill with unsafe file path" + files.push({ url: resource.href, dest }) + } + return { root, files } + } + + const planned: { root: string; files: { url: string; dest: string }[] }[] = [] + for (const skill of data.skills) { + const result = plan(skill) + if (typeof result === "string") yield* Effect.logWarning(result, { url: index, skill: skill.name }) + else planned.push(result) } // kilocode_change end const dirs = yield* Effect.forEach( - safe, // kilocode_change - was `list`; drop skills whose paths escape the cache + planned, // kilocode_change - validated, origin-pinned, cache-confined download plans (skill) => Effect.gen(function* () { - const root = path.join(cache, skill.name) - - yield* Effect.forEach( - skill.files, - (file) => download(new URL(file, `${host}/${skill.name}/`).href, path.join(root, file)), - { - concurrency: fileConcurrency, - }, - ) - - const md = path.join(root, "SKILL.md") - return (yield* fs.exists(md).pipe(Effect.orDie)) ? root : null + yield* Effect.forEach(skill.files, (file) => download(file.url, file.dest), { + concurrency: fileConcurrency, + }) + const md = path.join(skill.root, "SKILL.md") + return (yield* fs.exists(md).pipe(Effect.orDie)) ? skill.root : null }), { concurrency: skillConcurrency }, ) diff --git a/packages/opencode/test/skill/discovery.test.ts b/packages/opencode/test/skill/discovery.test.ts index ad1a53463f..f454a6b3a7 100644 --- a/packages/opencode/test/skill/discovery.test.ts +++ b/packages/opencode/test/skill/discovery.test.ts @@ -31,6 +31,10 @@ beforeAll(async () => { if (url.pathname.endsWith("/.agents/skills/evil/SKILL.md")) { return new Response("---\nname: evil\ndescription: evil.\n---\npwned") } + // A file entry pointing at another origin (exfil/arbitrary-host download) must be rejected. + if (url.pathname === "/cross-origin/index.json") { + return Response.json({ skills: [{ name: "x", files: ["SKILL.md", "https://evil.example/payload"] }] }) + } // kilocode_change end // route /.well-known/skills/* to the fixture directory @@ -135,6 +139,15 @@ describe("Discovery.pull", () => { expect(yield* fsys.existsSafe(escaped)).toBe(false) }), ) + + it.live("rejects a skill file that points at another origin", () => + Effect.gen(function* () { + const discovery = yield* Discovery.Service + // a file entry resolving to a different host must be dropped (no download, skill skipped) + const dirs = yield* discovery.pull(`http://localhost:${server.port}/cross-origin/`) + expect(dirs).toEqual([]) + }), + ) // kilocode_change end it.live("caches downloaded files on second pull", () =>