-
Notifications
You must be signed in to change notification settings - Fork 0
fix(security): add Linux PID tree walk and symlink containment for rules #84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import { realpath as fsRealpath } from "node:fs/promises" | ||
| import os from "os" | ||
| import path from "path" | ||
| import { Effect, Layer, ServiceMap } from "effect" | ||
|
|
@@ -144,21 +145,47 @@ export namespace Instruction { | |
| // Rules files from global and project directories | ||
| const globalRulesDir = path.join(Global.Path.home, ".opencode", "rules") | ||
|
|
||
| const globalRuleFiles = yield* fs | ||
| const rawGlobalRuleFiles = yield* fs | ||
| .glob("*.md", { cwd: globalRulesDir, absolute: true, include: "file" }) | ||
| .pipe(Effect.catch(() => Effect.succeed([] as string[]))) | ||
|
|
||
| // Symlink containment: reject files whose real path escapes the rules directory. | ||
| // This prevents a symlink like rules/evil.md -> ~/.ssh/id_rsa from injecting | ||
| // arbitrary content into the LLM context. | ||
| const filterSymlinkEscapes = async (files: string[], rulesDir: string): Promise<string[]> => { | ||
| const resolvedDir = await fsRealpath(rulesDir).catch(() => rulesDir) | ||
| const safe: string[] = [] | ||
| for (const file of files) { | ||
| try { | ||
| const resolvedPath = await fsRealpath(file) | ||
| if (resolvedPath.startsWith(resolvedDir + path.sep) || resolvedPath === resolvedDir) { | ||
| safe.push(file) | ||
| } | ||
|
Comment on lines
+155
to
+163
|
||
| } catch { | ||
| // broken symlink or permission error — skip | ||
| } | ||
| } | ||
| return safe | ||
| } | ||
|
|
||
| const globalRuleFiles = yield* Effect.promise(() => filterSymlinkEscapes(rawGlobalRuleFiles, globalRulesDir)) | ||
|
|
||
| // Project rules only load when project config is not disabled (trust boundary) | ||
| const projectRuleFiles = Flag.OPENCODE_DISABLE_PROJECT_CONFIG | ||
| const projectRulesDir = path.join(Instance.directory, ".opencode", "rules") | ||
| const rawProjectRuleFiles = Flag.OPENCODE_DISABLE_PROJECT_CONFIG | ||
| ? [] | ||
| : yield* fs | ||
| .glob("*.md", { | ||
| cwd: path.join(Instance.directory, ".opencode", "rules"), | ||
| cwd: projectRulesDir, | ||
| absolute: true, | ||
| include: "file", | ||
| }) | ||
| .pipe(Effect.catch(() => Effect.succeed([] as string[]))) | ||
|
|
||
| const projectRuleFiles = Flag.OPENCODE_DISABLE_PROJECT_CONFIG | ||
| ? [] | ||
| : yield* Effect.promise(() => filterSymlinkEscapes(rawProjectRuleFiles, projectRulesDir)) | ||
|
|
||
| // Project rules override global by filename | ||
| const projectFilenames = new Set(projectRuleFiles.map((p) => path.basename(p))) | ||
| for (const rule of globalRuleFiles) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| import { describe, expect, test } from "bun:test" | ||
| import { isAncestorPid } from "../../src/notification" | ||
|
|
||
| // Build a fake /proc/<pid>/stat line. Field index 3 (0-based) is the ppid. | ||
| // Real format: "pid (comm) S ppid pgrp session ..." | ||
| function procStat(pid: number, ppid: number, comm = "bash"): string { | ||
| return `${pid} (${comm}) S ${ppid} ${pid} ${pid} 0 -1 4194304` | ||
| } | ||
|
|
||
| type ReadFileFn = (path: string, encoding: "utf8") => Promise<string> | ||
|
|
||
| /** Helper: build a mock readFile from a pid->ppid map */ | ||
| function mockReadFile(tree: Record<number, number>): ReadFileFn { | ||
| return async (filePath: string) => { | ||
| const match = filePath.match(/\/proc\/(\d+)\/stat/) | ||
| if (!match) throw new Error("ENOENT") | ||
| const pid = parseInt(match[1], 10) | ||
| if (pid in tree) return procStat(pid, tree[pid]) | ||
| throw new Error("ENOENT: no such file or directory") | ||
| } | ||
| } | ||
|
|
||
| describe("isAncestorPid", () => { | ||
| test("returns true when ancestorPid matches currentPid directly", async () => { | ||
| // pid === ancestorPid on first iteration — no /proc read needed | ||
| const reader = mockReadFile({}) | ||
| const result = await isAncestorPid(100, 100, reader) | ||
| expect(result).toBe(true) | ||
| }) | ||
|
|
||
| test("returns true for direct parent", async () => { | ||
| // currentPid=200, parent=100 (the ancestor we're looking for) | ||
| const reader = mockReadFile({ 200: 100 }) | ||
| const result = await isAncestorPid(100, 200, reader) | ||
| expect(result).toBe(true) | ||
| }) | ||
|
|
||
| test("returns true for grandparent (two levels up)", async () => { | ||
| // currentPid=300 -> ppid=200 -> ppid=100 (ancestor) | ||
| const reader = mockReadFile({ 300: 200, 200: 100 }) | ||
| const result = await isAncestorPid(100, 300, reader) | ||
| expect(result).toBe(true) | ||
| }) | ||
|
|
||
| test("returns false when ancestorPid is not in the process tree", async () => { | ||
| // currentPid=300 -> ppid=200 -> ppid=1 (init, loop ends) | ||
| const reader = mockReadFile({ 300: 200, 200: 1 }) | ||
| const result = await isAncestorPid(999, 300, reader) | ||
| expect(result).toBe(false) | ||
| }) | ||
|
|
||
| test("handles cycle protection via visited set", async () => { | ||
| // Pathological case: pid 200 claims its parent is 200 (cycle) | ||
| const reader = mockReadFile({ 200: 200 }) | ||
| const result = await isAncestorPid(100, 200, reader) | ||
| expect(result).toBe(false) | ||
| }) | ||
|
|
||
| test("returns false gracefully when /proc is unavailable", async () => { | ||
| // Simulates macOS or any system without /proc | ||
| const reader: ReadFileFn = async () => { | ||
| throw new Error("ENOENT: no such file or directory") | ||
| } | ||
| const result = await isAncestorPid(100, 200, reader) | ||
| expect(result).toBe(false) | ||
| }) | ||
|
|
||
| test("returns false when /proc/stat contains non-numeric ppid", async () => { | ||
| const reader: ReadFileFn = async (filePath: string) => { | ||
| if (filePath === "/proc/200/stat") return "200 (bash) S notanumber 200 200 0" | ||
| throw new Error("ENOENT") | ||
| } | ||
| const result = await isAncestorPid(100, 200, reader) | ||
| expect(result).toBe(false) | ||
| }) | ||
|
|
||
| test("returns false when currentPid is 1 (init)", async () => { | ||
| // pid <= 1 should exit immediately without walking | ||
| const reader = mockReadFile({}) | ||
| const result = await isAncestorPid(100, 1, reader) | ||
| expect(result).toBe(false) | ||
| }) | ||
|
|
||
| test("handles process names with spaces (e.g. tmux: server)", async () => { | ||
| const reader: ReadFileFn = async (filePath: string) => { | ||
| if (filePath === "/proc/300/stat") | ||
| return "300 (tmux: server) S 200 300 300 0 -1 4194304" | ||
| if (filePath === "/proc/200/stat") | ||
| return "200 (bash) S 100 200 200 0 -1 4194304" | ||
| throw new Error("ENOENT") | ||
| } | ||
| expect(await isAncestorPid(100, 300, reader)).toBe(true) | ||
| }) | ||
|
|
||
| test("walks deep process trees correctly", async () => { | ||
| // currentPid=500 -> 400 -> 300 -> 200 -> 100 (ancestor) | ||
| const reader = mockReadFile({ 500: 400, 400: 300, 300: 200, 200: 100 }) | ||
| const result = await isAncestorPid(100, 500, reader) | ||
| expect(result).toBe(true) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,168 @@ | ||||||||||||||||||||||||||||||||||||||||||||||
| import { describe, expect, test } from "bun:test" | ||||||||||||||||||||||||||||||||||||||||||||||
| import * as fs from "fs/promises" | ||||||||||||||||||||||||||||||||||||||||||||||
| import path from "path" | ||||||||||||||||||||||||||||||||||||||||||||||
| import { Instruction } from "../../src/session/instruction" | ||||||||||||||||||||||||||||||||||||||||||||||
| import { Instance } from "../../src/project/instance" | ||||||||||||||||||||||||||||||||||||||||||||||
| import { tmpdir } from "../fixture/fixture" | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| describe("Instruction.systemPaths symlink containment", () => { | ||||||||||||||||||||||||||||||||||||||||||||||
| test("symlink escaping rules directory is excluded", async () => { | ||||||||||||||||||||||||||||||||||||||||||||||
| // Create a temp dir with a rules directory containing: | ||||||||||||||||||||||||||||||||||||||||||||||
| // - normal.md (regular file, should be included) | ||||||||||||||||||||||||||||||||||||||||||||||
| // - evil.md -> /etc/hosts (symlink escape, should be excluded) | ||||||||||||||||||||||||||||||||||||||||||||||
| await using homeTmp = await tmpdir({ | ||||||||||||||||||||||||||||||||||||||||||||||
| init: async (dir) => { | ||||||||||||||||||||||||||||||||||||||||||||||
| const rulesDir = path.join(dir, ".opencode", "rules") | ||||||||||||||||||||||||||||||||||||||||||||||
| await fs.mkdir(rulesDir, { recursive: true }) | ||||||||||||||||||||||||||||||||||||||||||||||
| await Bun.write(path.join(rulesDir, "normal.md"), "# Normal Rules") | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| // Create a symlink that escapes the rules directory | ||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||
| await fs.symlink("/etc/hosts", path.join(rulesDir, "evil.md")) | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+12
to
+21
|
||||||||||||||||||||||||||||||||||||||||||||||
| // - evil.md -> /etc/hosts (symlink escape, should be excluded) | |
| await using homeTmp = await tmpdir({ | |
| init: async (dir) => { | |
| const rulesDir = path.join(dir, ".opencode", "rules") | |
| await fs.mkdir(rulesDir, { recursive: true }) | |
| await Bun.write(path.join(rulesDir, "normal.md"), "# Normal Rules") | |
| // Create a symlink that escapes the rules directory | |
| try { | |
| await fs.symlink("/etc/hosts", path.join(rulesDir, "evil.md")) | |
| // - evil.md -> file outside the rules dir (symlink escape, should be excluded) | |
| await using homeTmp = await tmpdir({ | |
| init: async (dir) => { | |
| const rulesDir = path.join(dir, ".opencode", "rules") | |
| const outsideFile = path.join(dir, "outside.md") | |
| await fs.mkdir(rulesDir, { recursive: true }) | |
| await Bun.write(path.join(rulesDir, "normal.md"), "# Normal Rules") | |
| await Bun.write(outsideFile, "# Outside Rules") | |
| // Create a symlink that resolves to a real file outside the rules directory | |
| try { | |
| await fs.symlink(outsideFile, path.join(rulesDir, "evil.md")) |
Copilot
AI
Apr 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test can pass even if the symlink creation fails (the catch {} silently ignores it), making the assertion about evil.md vacuous. Instead, detect whether the symlink was created (e.g., via lstat/readlink) and test.skip()/early-return when symlinks aren't supported, or fail the test when symlink creation is expected.
Copilot
AI
Apr 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue as above: the catch {} makes this test potentially vacuous on platforms/environments where symlinks can't be created. Please explicitly skip or assert symlink support/creation so CI meaningfully exercises containment behavior.
Copilot
AI
Apr 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue as above: if fs.symlink fails, the test still passes without validating broken-symlink handling. Consider skipping when symlinks aren't available, and asserting the broken symlink entry exists before calling Instruction.systemPaths().
Copilot
AI
Apr 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue as above: ignoring symlink creation failure can make this project-escape test pass without actually creating sneaky.md. Please skip or assert symlink creation so the security behavior is exercised on supported platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/proc/<pid>/statparsing viastat.split(" ")[3]is not robust: the(comm)field can contain spaces, which shifts the split indices and can produce an incorrect PPID. Consider parsing by locating the closing)of the comm field and then splitting the remainder, or using a regex that extracts the PPID after) <state>.