fix(cli): confine downloaded skills to the cache directory

This commit is contained in:
Bruno Agatao
2026-07-29 12:27:21 +02:00
parent cd266e8e55
commit 3cef158ef0
2 changed files with 38 additions and 1 deletions
+15 -1
View File
@@ -71,8 +71,22 @@ export const layer: Layer.Layer<Service, never, FSUtil.Service | Path.Path | Htt
)
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)
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)
@@ -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