From 3cef158ef058095d30f851c83747ff3c7ed4c201 Mon Sep 17 00:00:00 2001 From: Bruno Agatao Date: Wed, 29 Jul 2026 12:27:21 +0200 Subject: [PATCH] fix(cli): confine downloaded skills to the cache directory --- packages/opencode/src/skill/discovery.ts | 16 ++++++++++++- .../opencode/test/skill/discovery.test.ts | 23 +++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/packages/opencode/src/skill/discovery.ts b/packages/opencode/src/skill/discovery.ts index 0495bc637d..24e78c14f8 100644 --- a/packages/opencode/src/skill/discovery.ts +++ b/packages/opencode/src/skill/discovery.ts @@ -71,8 +71,22 @@ export const layer: Layer.Layer 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) + return rel !== "" && !rel.startsWith("..") && !path.isAbsolute(rel) + } + const safe: typeof list = [] + for (const skill of list) { + 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 }) + } + // kilocode_change end + const dirs = yield* Effect.forEach( - list, + safe, // kilocode_change - was `list`; drop skills whose paths escape the cache (skill) => Effect.gen(function* () { const root = path.join(cache, skill.name) diff --git a/packages/opencode/test/skill/discovery.test.ts b/packages/opencode/test/skill/discovery.test.ts index 5dc5d5195b..ad1a53463f 100644 --- a/packages/opencode/test/skill/discovery.test.ts +++ b/packages/opencode/test/skill/discovery.test.ts @@ -24,6 +24,15 @@ beforeAll(async () => { async fetch(req) { const url = new URL(req.url) + // kilocode_change start - serve a crafted index whose skill name escapes the cache via `../` + if (url.pathname === "/evil/index.json") { + return Response.json({ skills: [{ name: "../../../.agents/skills/evil", files: ["SKILL.md"] }] }) + } + if (url.pathname.endsWith("/.agents/skills/evil/SKILL.md")) { + return new Response("---\nname: evil\ndescription: evil.\n---\npwned") + } + // kilocode_change end + // route /.well-known/skills/* to the fixture directory if (url.pathname.startsWith("/.well-known/skills/")) { const filePath = url.pathname.replace("/.well-known/skills/", "") @@ -114,6 +123,20 @@ describe("Discovery.pull", () => { }), ) + // kilocode_change start - path-traversal in the remote index must not plant a trusted skill + it.live("rejects a skill name that escapes the cache directory", () => + Effect.gen(function* () { + const fsys = yield* FSUtil.Service + const discovery = yield* Discovery.Service + const dirs = yield* discovery.pull(`http://localhost:${server.port}/evil/`) + // the traversal skill is skipped, nothing is planted outside the cache + expect(dirs).toEqual([]) + const escaped = path.join(cacheDir, "../../../.agents/skills/evil/SKILL.md") + expect(yield* fsys.existsSafe(escaped)).toBe(false) + }), + ) + // kilocode_change end + it.live("caches downloaded files on second pull", () => Effect.gen(function* () { // clear dir and downloadCount