fix(security): confine file resolution to project roots
Block traversal, out-of-root absolute path, and symlink escapes for @file references, file:// URIs, and config skill file loading while logging rejected attempts. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
This commit is contained in:
@@ -0,0 +1,88 @@
|
||||
import { afterAll, beforeAll, describe, expect, test } from "bun:test"
|
||||
import { mkdirSync, rmSync, symlinkSync, writeFileSync } from "node:fs"
|
||||
import { tmpdir } from "node:os"
|
||||
import { join } from "node:path"
|
||||
import type { SkillDefinition } from "../../../config/schema"
|
||||
import { configEntryToLoadedSkill } from "./config-skill-entry-loader"
|
||||
|
||||
describe("configEntryToLoadedSkill", () => {
|
||||
const fixtureRoot = join(tmpdir(), `config-skill-entry-loader-${Date.now()}`)
|
||||
const configDir = join(fixtureRoot, "config")
|
||||
const allowedSkillPath = join(configDir, "allowed-skill.md")
|
||||
const linkedSecretSkillPath = join(configDir, "linked-secret-skill.md")
|
||||
const outsideSkillPath = join(fixtureRoot, "secret-skill.md")
|
||||
|
||||
beforeAll(() => {
|
||||
mkdirSync(configDir, { recursive: true })
|
||||
writeFileSync(
|
||||
allowedSkillPath,
|
||||
[
|
||||
"---",
|
||||
"description: Allowed skill",
|
||||
"---",
|
||||
"Use ./allowed.txt for context.",
|
||||
].join("\n"),
|
||||
"utf8"
|
||||
)
|
||||
writeFileSync(
|
||||
outsideSkillPath,
|
||||
[
|
||||
"---",
|
||||
"description: Secret skill",
|
||||
"---",
|
||||
"Do not leak this.",
|
||||
].join("\n"),
|
||||
"utf8"
|
||||
)
|
||||
symlinkSync(outsideSkillPath, linkedSecretSkillPath)
|
||||
})
|
||||
|
||||
afterAll(() => {
|
||||
rmSync(fixtureRoot, { recursive: true, force: true })
|
||||
})
|
||||
|
||||
test("loads skills from files within configDir", () => {
|
||||
//#given
|
||||
const entry: SkillDefinition = { from: "./allowed-skill.md" }
|
||||
|
||||
//#when
|
||||
const loaded = configEntryToLoadedSkill("allowed-skill", entry, configDir)
|
||||
|
||||
//#then
|
||||
expect(loaded).not.toBeNull()
|
||||
expect(loaded?.definition.template).toContain("Use ./allowed.txt for context.")
|
||||
})
|
||||
|
||||
test("rejects absolute skill files outside configDir", () => {
|
||||
//#given
|
||||
const entry: SkillDefinition = { from: outsideSkillPath }
|
||||
|
||||
//#when
|
||||
const loaded = configEntryToLoadedSkill("secret-skill", entry, configDir)
|
||||
|
||||
//#then
|
||||
expect(loaded).toBeNull()
|
||||
})
|
||||
|
||||
test("rejects traversal skill files that escape configDir", () => {
|
||||
//#given
|
||||
const entry: SkillDefinition = { from: "../secret-skill.md" }
|
||||
|
||||
//#when
|
||||
const loaded = configEntryToLoadedSkill("secret-skill", entry, configDir)
|
||||
|
||||
//#then
|
||||
expect(loaded).toBeNull()
|
||||
})
|
||||
|
||||
test("rejects symlink skill files that escape configDir", () => {
|
||||
//#given
|
||||
const entry: SkillDefinition = { from: "./linked-secret-skill.md" }
|
||||
|
||||
//#when
|
||||
const loaded = configEntryToLoadedSkill("secret-skill", entry, configDir)
|
||||
|
||||
//#then
|
||||
expect(loaded).toBeNull()
|
||||
})
|
||||
})
|
||||
@@ -5,6 +5,8 @@ import { existsSync, readFileSync } from "fs"
|
||||
import { dirname, isAbsolute, resolve } from "path"
|
||||
import { homedir } from "os"
|
||||
import { parseFrontmatter } from "../../../shared/frontmatter"
|
||||
import { isWithinProject } from "../../../shared/contains-path"
|
||||
import { log } from "../../../shared/logger"
|
||||
import { sanitizeModelField } from "../../../shared/model-sanitizer"
|
||||
import { resolveSkillPathReferences } from "../../../shared/skill-path-resolver"
|
||||
import { parseAllowedTools } from "../allowed-tools-parser"
|
||||
@@ -46,10 +48,22 @@ export function configEntryToLoadedSkill(
|
||||
): LoadedSkill | null {
|
||||
let template = entry.template || ""
|
||||
let fileMetadata: SkillMetadata = {}
|
||||
let sourcePath: string | undefined
|
||||
|
||||
if (entry.from) {
|
||||
const filePath = resolveFilePath(entry.from, configDir)
|
||||
const loaded = loadSkillFromFile(filePath)
|
||||
sourcePath = resolveFilePath(entry.from, configDir)
|
||||
const projectRoot = configDir || process.cwd()
|
||||
|
||||
if (!isWithinProject(sourcePath, projectRoot)) {
|
||||
log("[config-skill-entry-loader] Rejected skill entry file outside project root", {
|
||||
from: entry.from,
|
||||
filePath: sourcePath,
|
||||
projectRoot,
|
||||
})
|
||||
return null
|
||||
}
|
||||
|
||||
const loaded = loadSkillFromFile(sourcePath)
|
||||
if (loaded) {
|
||||
template = loaded.template
|
||||
fileMetadata = loaded.metadata
|
||||
@@ -63,9 +77,7 @@ export function configEntryToLoadedSkill(
|
||||
}
|
||||
|
||||
const description = entry.description || fileMetadata.description || ""
|
||||
const resolvedPath = entry.from
|
||||
? dirname(resolveFilePath(entry.from, configDir))
|
||||
: configDir || process.cwd()
|
||||
const resolvedPath = sourcePath ? dirname(sourcePath) : configDir || process.cwd()
|
||||
|
||||
const resolvedTemplate = resolveSkillPathReferences(template.trim(), resolvedPath)
|
||||
const wrappedTemplate = `<skill-instruction>
|
||||
@@ -93,7 +105,7 @@ $ARGUMENTS
|
||||
|
||||
return {
|
||||
name,
|
||||
path: entry.from ? resolveFilePath(entry.from, configDir) : undefined,
|
||||
path: sourcePath,
|
||||
resolvedPath,
|
||||
definition,
|
||||
scope: "config",
|
||||
|
||||
Reference in New Issue
Block a user