From 9a3b3b6baf01a0e4148b6e6391976f2b2ac61d09 Mon Sep 17 00:00:00 2001 From: Terada Kousuke Date: Sun, 5 Apr 2026 21:30:50 +0900 Subject: [PATCH 1/2] fix(security): add Linux PID tree walk and symlink containment for rules (#73) Problem A: xdotool returns the terminal emulator PID which is an ancestor further up the process tree, not the direct parent. The flat comparison against process.pid/process.ppid almost always returned false. Now walks /proc//stat upward to find the ancestor relationship. Problem B: symlinks in ~/.opencode/rules/ or project rules/ could escape the directory (e.g. evil.md -> ~/.ssh/id_rsa), injecting arbitrary file contents into LLM context. Now validates via fs.realpath() that each resolved path stays within its rules directory. Co-Authored-By: Claude Opus 4.6 (1M context) --- packages/opencode/src/notification/index.ts | 35 +++- packages/opencode/src/session/instruction.ts | 33 +++- .../test/notification/pid-ancestor.test.ts | 90 ++++++++++ .../test/session/rules-symlink.test.ts | 168 ++++++++++++++++++ 4 files changed, 322 insertions(+), 4 deletions(-) create mode 100644 packages/opencode/test/notification/pid-ancestor.test.ts create mode 100644 packages/opencode/test/session/rules-symlink.test.ts diff --git a/packages/opencode/src/notification/index.ts b/packages/opencode/src/notification/index.ts index ad4e339f2392..bc0dc801047e 100644 --- a/packages/opencode/src/notification/index.ts +++ b/packages/opencode/src/notification/index.ts @@ -1,3 +1,4 @@ +import { readFile } from "node:fs/promises" import { platform } from "os" import { Process } from "@/util/process" import { which } from "@/util/which" @@ -26,6 +27,38 @@ const APPS = new Set([ "windsurf", ]) +type ReadFileFn = (path: string, encoding: "utf8") => Promise + +/** + * Walk the process tree via /proc//stat to check whether `ancestorPid` + * is an ancestor of `currentPid`. On non-Linux systems (where /proc is + * unavailable) this gracefully returns false. + * + * The optional `readFileFn` parameter allows tests to inject a mock without + * patching the global `node:fs/promises` module. + */ +export async function isAncestorPid( + ancestorPid: number, + currentPid: number, + readFileFn: ReadFileFn = readFile as ReadFileFn, +): Promise { + let pid = currentPid + const visited = new Set() + while (pid > 1 && !visited.has(pid)) { + if (pid === ancestorPid) return true + visited.add(pid) + try { + const stat = await readFileFn(`/proc/${pid}/stat`, "utf8") + const ppid = parseInt(stat.split(" ")[3], 10) + if (isNaN(ppid)) break + pid = ppid + } catch { + break + } + } + return false +} + function norm(s: string) { return s.toLowerCase().replace(/[^a-z0-9]/g, "") } @@ -68,7 +101,7 @@ export namespace Notification { if (result.code !== 0) return true const pid = parseInt(result.text.trim(), 10) if (isNaN(pid)) return true - return pid === process.pid || pid === process.ppid + return await isAncestorPid(pid, process.pid) } if (os === "win32") return false diff --git a/packages/opencode/src/session/instruction.ts b/packages/opencode/src/session/instruction.ts index d22a640fd11c..039b14cbd853 100644 --- a/packages/opencode/src/session/instruction.ts +++ b/packages/opencode/src/session/instruction.ts @@ -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 => { + 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) + } + } 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) { diff --git a/packages/opencode/test/notification/pid-ancestor.test.ts b/packages/opencode/test/notification/pid-ancestor.test.ts new file mode 100644 index 000000000000..4fcfc75a974a --- /dev/null +++ b/packages/opencode/test/notification/pid-ancestor.test.ts @@ -0,0 +1,90 @@ +import { describe, expect, test } from "bun:test" +import { isAncestorPid } from "../../src/notification" + +// Build a fake /proc//stat line. Field index 3 (0-based) is the ppid. +// Real format: "pid (comm) S ppid pgrp session ..." +function procStat(pid: number, ppid: number): string { + return `${pid} (bash) S ${ppid} ${pid} ${pid} 0 -1 4194304` +} + +type ReadFileFn = (path: string, encoding: "utf8") => Promise + +/** Helper: build a mock readFile from a pid->ppid map */ +function mockReadFile(tree: Record): 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("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) + }) +}) diff --git a/packages/opencode/test/session/rules-symlink.test.ts b/packages/opencode/test/session/rules-symlink.test.ts new file mode 100644 index 000000000000..369f3a7088aa --- /dev/null +++ b/packages/opencode/test/session/rules-symlink.test.ts @@ -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")) + } catch { + // If we can't create symlinks (permission issue), skip this part + } + }, + }) + await using projectTmp = await tmpdir() + + const originalHome = process.env.OPENCODE_TEST_HOME + process.env.OPENCODE_TEST_HOME = homeTmp.path + + try { + await Instance.provide({ + directory: projectTmp.path, + fn: async () => { + const paths = await Instruction.systemPaths() + const allPaths = Array.from(paths) + + // Normal file should be included + expect(allPaths.some((p) => p.endsWith("normal.md"))).toBe(true) + + // Symlink escape should be excluded + expect(allPaths.some((p) => p.endsWith("evil.md"))).toBe(false) + }, + }) + } finally { + process.env.OPENCODE_TEST_HOME = originalHome + } + }) + + test("symlink within rules directory is allowed", async () => { + // A symlink that points to another file INSIDE the rules dir should be fine + 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, "original.md"), "# Original Rules") + + try { + await fs.symlink( + path.join(rulesDir, "original.md"), + path.join(rulesDir, "alias.md"), + ) + } catch { + // If symlinks not supported, test is meaningless + } + }, + }) + await using projectTmp = await tmpdir() + + const originalHome = process.env.OPENCODE_TEST_HOME + process.env.OPENCODE_TEST_HOME = homeTmp.path + + try { + await Instance.provide({ + directory: projectTmp.path, + fn: async () => { + const paths = await Instruction.systemPaths() + const allPaths = Array.from(paths) + + // Both original and alias should be included (alias resolves within rules dir) + expect(allPaths.some((p) => p.endsWith("original.md"))).toBe(true) + expect(allPaths.some((p) => p.endsWith("alias.md"))).toBe(true) + }, + }) + } finally { + process.env.OPENCODE_TEST_HOME = originalHome + } + }) + + test("broken symlink is skipped gracefully", async () => { + 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, "valid.md"), "# Valid Rules") + + try { + // Symlink to a non-existent target + await fs.symlink("/nonexistent/path/to/nowhere.md", path.join(rulesDir, "broken.md")) + } catch { + // If symlinks not supported, test is meaningless + } + }, + }) + await using projectTmp = await tmpdir() + + const originalHome = process.env.OPENCODE_TEST_HOME + process.env.OPENCODE_TEST_HOME = homeTmp.path + + try { + await Instance.provide({ + directory: projectTmp.path, + fn: async () => { + // Should not throw + const paths = await Instruction.systemPaths() + const allPaths = Array.from(paths) + + // Valid file should be included + expect(allPaths.some((p) => p.endsWith("valid.md"))).toBe(true) + + // Broken symlink should be excluded (realpath fails) + expect(allPaths.some((p) => p.endsWith("broken.md"))).toBe(false) + }, + }) + } finally { + process.env.OPENCODE_TEST_HOME = originalHome + } + }) + + test("project rules symlink escape is also blocked", async () => { + await using homeTmp = await tmpdir() + await using projectTmp = await tmpdir({ + init: async (dir) => { + const rulesDir = path.join(dir, ".opencode", "rules") + await fs.mkdir(rulesDir, { recursive: true }) + await Bun.write(path.join(rulesDir, "legit.md"), "# Legit Project Rules") + + try { + await fs.symlink("/etc/passwd", path.join(rulesDir, "sneaky.md")) + } catch { + // If symlinks not supported + } + }, + }) + + const originalHome = process.env.OPENCODE_TEST_HOME + process.env.OPENCODE_TEST_HOME = homeTmp.path + + try { + await Instance.provide({ + directory: projectTmp.path, + fn: async () => { + const paths = await Instruction.systemPaths() + const allPaths = Array.from(paths) + + // Legit file should be included + expect(allPaths.some((p) => p.endsWith("legit.md"))).toBe(true) + + // Symlink escape should be excluded + expect(allPaths.some((p) => p.endsWith("sneaky.md"))).toBe(false) + }, + }) + } finally { + process.env.OPENCODE_TEST_HOME = originalHome + } + }) +}) From c250834992a8a39d0bce7dda4680357e885a6d3f Mon Sep 17 00:00:00 2001 From: Terada Kousuke Date: Sun, 5 Apr 2026 21:54:54 +0900 Subject: [PATCH 2/2] fix(security): use lastIndexOf for safe /proc/pid/stat comm field parsing (#84) The comm field (field 2) in /proc/[pid]/stat is wrapped in parentheses and may contain spaces (e.g. "tmux: server"), which broke the naive split(" ")[3] approach. Parse after the last ")" to safely extract ppid. Co-Authored-By: Claude Opus 4.6 (1M context) --- packages/opencode/src/notification/index.ts | 8 +++++++- .../test/notification/pid-ancestor.test.ts | 15 +++++++++++++-- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/packages/opencode/src/notification/index.ts b/packages/opencode/src/notification/index.ts index bc0dc801047e..5b8b275024c2 100644 --- a/packages/opencode/src/notification/index.ts +++ b/packages/opencode/src/notification/index.ts @@ -49,7 +49,13 @@ export async function isAncestorPid( visited.add(pid) try { const stat = await readFileFn(`/proc/${pid}/stat`, "utf8") - const ppid = parseInt(stat.split(" ")[3], 10) + // The comm field is wrapped in parentheses and may contain spaces. + // The only safe delimiter is the last ')' in the line. + const closeParenIdx = stat.lastIndexOf(")") + if (closeParenIdx === -1) break + const afterComm = stat.substring(closeParenIdx + 2) // skip ") " + const fields = afterComm.split(" ") + const ppid = parseInt(fields[1], 10) // fields: [state, ppid, pgrp, ...] if (isNaN(ppid)) break pid = ppid } catch { diff --git a/packages/opencode/test/notification/pid-ancestor.test.ts b/packages/opencode/test/notification/pid-ancestor.test.ts index 4fcfc75a974a..b398e93c8b34 100644 --- a/packages/opencode/test/notification/pid-ancestor.test.ts +++ b/packages/opencode/test/notification/pid-ancestor.test.ts @@ -3,8 +3,8 @@ import { isAncestorPid } from "../../src/notification" // Build a fake /proc//stat line. Field index 3 (0-based) is the ppid. // Real format: "pid (comm) S ppid pgrp session ..." -function procStat(pid: number, ppid: number): string { - return `${pid} (bash) S ${ppid} ${pid} ${pid} 0 -1 4194304` +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 @@ -81,6 +81,17 @@ describe("isAncestorPid", () => { 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 })