fix(write-existing-file-guard): wire cleanup through event dispatcher
Forward session.deleted events to write-existing-file-guard so per-session read permissions are actually cleared in runtime. Add plugin-level regression test to ensure event forwarding remains wired, alongside the expanded guard behavior and unit coverage.
This commit is contained in:
parent
6d5d250f8f
commit
73d9e1f847
@ -1,7 +1,7 @@
|
|||||||
import type { Hooks, PluginInput } from "@opencode-ai/plugin"
|
import type { Hooks, PluginInput } from "@opencode-ai/plugin"
|
||||||
|
|
||||||
import { existsSync, realpathSync } from "fs"
|
import { existsSync, realpathSync } from "fs"
|
||||||
import { basename, dirname, isAbsolute, join, normalize, resolve, sep } from "path"
|
import { basename, dirname, isAbsolute, join, normalize, relative, resolve, sep } from "path"
|
||||||
|
|
||||||
import { log } from "../../shared"
|
import { log } from "../../shared"
|
||||||
|
|
||||||
@ -13,9 +13,11 @@ type GuardArgs = {
|
|||||||
}
|
}
|
||||||
|
|
||||||
const MAX_TRACKED_SESSIONS = 256
|
const MAX_TRACKED_SESSIONS = 256
|
||||||
|
export const MAX_TRACKED_PATHS_PER_SESSION = 1024
|
||||||
|
const OUTSIDE_SESSION_MESSAGE = "Path must be inside session directory."
|
||||||
|
|
||||||
function asRecord(value: unknown): Record<string, unknown> | undefined {
|
function asRecord(value: unknown): Record<string, unknown> | undefined {
|
||||||
if (!value || typeof value !== "object") {
|
if (!value || typeof value !== "object" || Array.isArray(value)) {
|
||||||
return undefined
|
return undefined
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -30,6 +32,11 @@ function resolveInputPath(ctx: PluginInput, inputPath: string): string {
|
|||||||
return normalize(isAbsolute(inputPath) ? inputPath : resolve(ctx.directory, inputPath))
|
return normalize(isAbsolute(inputPath) ? inputPath : resolve(ctx.directory, inputPath))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function isPathInsideDirectory(pathToCheck: string, directory: string): boolean {
|
||||||
|
const relativePath = relative(directory, pathToCheck)
|
||||||
|
return relativePath === "" || (!relativePath.startsWith("..") && !isAbsolute(relativePath))
|
||||||
|
}
|
||||||
|
|
||||||
function toCanonicalPath(absolutePath: string): string {
|
function toCanonicalPath(absolutePath: string): string {
|
||||||
let canonicalPath = absolutePath
|
let canonicalPath = absolutePath
|
||||||
|
|
||||||
@ -106,9 +113,25 @@ export function createWriteExistingFileGuardHook(ctx: PluginInput): Hooks {
|
|||||||
return readSet
|
return readSet
|
||||||
}
|
}
|
||||||
|
|
||||||
|
const trimSessionReadSet = (readSet: Set<string>): void => {
|
||||||
|
while (readSet.size > MAX_TRACKED_PATHS_PER_SESSION) {
|
||||||
|
const oldestPath = readSet.values().next().value
|
||||||
|
if (!oldestPath) {
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
|
readSet.delete(oldestPath)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
const registerReadPermission = (sessionID: string, canonicalPath: string): void => {
|
const registerReadPermission = (sessionID: string, canonicalPath: string): void => {
|
||||||
const readSet = ensureSessionReadSet(sessionID)
|
const readSet = ensureSessionReadSet(sessionID)
|
||||||
|
if (readSet.has(canonicalPath)) {
|
||||||
|
readSet.delete(canonicalPath)
|
||||||
|
}
|
||||||
|
|
||||||
readSet.add(canonicalPath)
|
readSet.add(canonicalPath)
|
||||||
|
trimSessionReadSet(readSet)
|
||||||
}
|
}
|
||||||
|
|
||||||
const consumeReadPermission = (sessionID: string, canonicalPath: string): boolean => {
|
const consumeReadPermission = (sessionID: string, canonicalPath: string): boolean => {
|
||||||
@ -128,9 +151,7 @@ export function createWriteExistingFileGuardHook(ctx: PluginInput): Hooks {
|
|||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
|
|
||||||
if (readSet.delete(canonicalPath)) {
|
readSet.delete(canonicalPath)
|
||||||
touchSession(sessionID)
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -150,6 +171,20 @@ export function createWriteExistingFileGuardHook(ctx: PluginInput): Hooks {
|
|||||||
|
|
||||||
const resolvedPath = resolveInputPath(ctx, filePath)
|
const resolvedPath = resolveInputPath(ctx, filePath)
|
||||||
const canonicalPath = toCanonicalPath(resolvedPath)
|
const canonicalPath = toCanonicalPath(resolvedPath)
|
||||||
|
const isInsideSessionDirectory = isPathInsideDirectory(canonicalPath, canonicalSessionRoot)
|
||||||
|
|
||||||
|
if (!isInsideSessionDirectory) {
|
||||||
|
if (toolName === "read") {
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
|
log("[write-existing-file-guard] Blocking write outside session directory", {
|
||||||
|
sessionID: input.sessionID,
|
||||||
|
filePath,
|
||||||
|
resolvedPath,
|
||||||
|
})
|
||||||
|
throw new Error(OUTSIDE_SESSION_MESSAGE)
|
||||||
|
}
|
||||||
|
|
||||||
if (toolName === "read") {
|
if (toolName === "read") {
|
||||||
if (!existsSync(resolvedPath) || !input.sessionID) {
|
if (!existsSync(resolvedPath) || !input.sessionID) {
|
||||||
@ -163,6 +198,7 @@ export function createWriteExistingFileGuardHook(ctx: PluginInput): Hooks {
|
|||||||
const overwriteEnabled = isOverwriteEnabled(args?.overwrite)
|
const overwriteEnabled = isOverwriteEnabled(args?.overwrite)
|
||||||
|
|
||||||
if (argsRecord && "overwrite" in argsRecord) {
|
if (argsRecord && "overwrite" in argsRecord) {
|
||||||
|
// Intentionally mutate output args so overwrite bypass remains hook-only.
|
||||||
delete argsRecord.overwrite
|
delete argsRecord.overwrite
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -208,5 +244,19 @@ export function createWriteExistingFileGuardHook(ctx: PluginInput): Hooks {
|
|||||||
|
|
||||||
throw new Error("File already exists. Use edit tool instead.")
|
throw new Error("File already exists. Use edit tool instead.")
|
||||||
},
|
},
|
||||||
|
event: async ({ event }: { event: { type: string; properties?: unknown } }) => {
|
||||||
|
if (event.type !== "session.deleted") {
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
|
const props = event.properties as { info?: { id?: string } } | undefined
|
||||||
|
const sessionID = props?.info?.id
|
||||||
|
if (!sessionID) {
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
|
readPermissionsBySession.delete(sessionID)
|
||||||
|
sessionLastAccess.delete(sessionID)
|
||||||
|
},
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@ -3,9 +3,11 @@ import { existsSync, mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync
|
|||||||
import { tmpdir } from "node:os"
|
import { tmpdir } from "node:os"
|
||||||
import { dirname, join, resolve } from "node:path"
|
import { dirname, join, resolve } from "node:path"
|
||||||
|
|
||||||
|
import { MAX_TRACKED_PATHS_PER_SESSION } from "./hook"
|
||||||
import { createWriteExistingFileGuardHook } from "./index"
|
import { createWriteExistingFileGuardHook } from "./index"
|
||||||
|
|
||||||
const BLOCK_MESSAGE = "File already exists. Use edit tool instead."
|
const BLOCK_MESSAGE = "File already exists. Use edit tool instead."
|
||||||
|
const OUTSIDE_SESSION_MESSAGE = "Path must be inside session directory."
|
||||||
|
|
||||||
type Hook = ReturnType<typeof createWriteExistingFileGuardHook>
|
type Hook = ReturnType<typeof createWriteExistingFileGuardHook>
|
||||||
|
|
||||||
@ -54,6 +56,10 @@ describe("createWriteExistingFileGuardHook", () => {
|
|||||||
return output
|
return output
|
||||||
}
|
}
|
||||||
|
|
||||||
|
const emitSessionDeleted = async (sessionID: string): Promise<void> => {
|
||||||
|
await hook.event?.({ event: { type: "session.deleted", properties: { info: { id: sessionID } } } })
|
||||||
|
}
|
||||||
|
|
||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
tempDir = mkdtempSync(join(tmpdir(), "write-existing-file-guard-"))
|
tempDir = mkdtempSync(join(tmpdir(), "write-existing-file-guard-"))
|
||||||
hook = createWriteExistingFileGuardHook({ directory: tempDir } as never)
|
hook = createWriteExistingFileGuardHook({ directory: tempDir } as never)
|
||||||
@ -111,6 +117,39 @@ describe("createWriteExistingFileGuardHook", () => {
|
|||||||
).rejects.toThrow(BLOCK_MESSAGE)
|
).rejects.toThrow(BLOCK_MESSAGE)
|
||||||
})
|
})
|
||||||
|
|
||||||
|
test("#given same-session concurrent writes #when only one read permission exists #then allows only one write", async () => {
|
||||||
|
const existingFile = createFile("concurrent-consume.txt")
|
||||||
|
const sessionID = "ses_concurrent"
|
||||||
|
|
||||||
|
await invoke({
|
||||||
|
tool: "read",
|
||||||
|
sessionID,
|
||||||
|
outputArgs: { filePath: existingFile },
|
||||||
|
})
|
||||||
|
|
||||||
|
const results = await Promise.allSettled([
|
||||||
|
invoke({
|
||||||
|
tool: "write",
|
||||||
|
sessionID,
|
||||||
|
outputArgs: { filePath: existingFile, content: "first attempt" },
|
||||||
|
}),
|
||||||
|
invoke({
|
||||||
|
tool: "write",
|
||||||
|
sessionID,
|
||||||
|
outputArgs: { filePath: existingFile, content: "second attempt" },
|
||||||
|
}),
|
||||||
|
])
|
||||||
|
|
||||||
|
const successCount = results.filter((result) => result.status === "fulfilled").length
|
||||||
|
const failures = results.filter(
|
||||||
|
(result): result is PromiseRejectedResult => result.status === "rejected"
|
||||||
|
)
|
||||||
|
|
||||||
|
expect(successCount).toBe(1)
|
||||||
|
expect(failures).toHaveLength(1)
|
||||||
|
expect(String(failures[0]?.reason)).toContain(BLOCK_MESSAGE)
|
||||||
|
})
|
||||||
|
|
||||||
test("#given read in another session #when write executes #then blocks", async () => {
|
test("#given read in another session #when write executes #then blocks", async () => {
|
||||||
const existingFile = createFile("cross-session.txt")
|
const existingFile = createFile("cross-session.txt")
|
||||||
|
|
||||||
@ -159,6 +198,23 @@ describe("createWriteExistingFileGuardHook", () => {
|
|||||||
expect(output.args.overwrite).toBeUndefined()
|
expect(output.args.overwrite).toBeUndefined()
|
||||||
})
|
})
|
||||||
|
|
||||||
|
test("#given overwrite falsy values #when write executes #then does not bypass guard", async () => {
|
||||||
|
const existingFile = createFile("overwrite-falsy.txt")
|
||||||
|
|
||||||
|
for (const overwrite of [false, "false"] as const) {
|
||||||
|
await expect(
|
||||||
|
invoke({
|
||||||
|
tool: "write",
|
||||||
|
outputArgs: {
|
||||||
|
filePath: existingFile,
|
||||||
|
content: "new content",
|
||||||
|
overwrite,
|
||||||
|
},
|
||||||
|
})
|
||||||
|
).rejects.toThrow(BLOCK_MESSAGE)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
|
||||||
test("#given two sessions read same file #when one writes #then other session is invalidated", async () => {
|
test("#given two sessions read same file #when one writes #then other session is invalidated", async () => {
|
||||||
const existingFile = createFile("invalidate.txt")
|
const existingFile = createFile("invalidate.txt")
|
||||||
|
|
||||||
@ -227,6 +283,41 @@ describe("createWriteExistingFileGuardHook", () => {
|
|||||||
}
|
}
|
||||||
})
|
})
|
||||||
|
|
||||||
|
test("#given tools without file path arg #when write and read execute #then ignores safely", async () => {
|
||||||
|
await expect(
|
||||||
|
invoke({
|
||||||
|
tool: "write",
|
||||||
|
outputArgs: { content: "no path" },
|
||||||
|
})
|
||||||
|
).resolves.toBeDefined()
|
||||||
|
|
||||||
|
await expect(
|
||||||
|
invoke({
|
||||||
|
tool: "read",
|
||||||
|
outputArgs: {},
|
||||||
|
})
|
||||||
|
).resolves.toBeDefined()
|
||||||
|
})
|
||||||
|
|
||||||
|
test("#given non-read-write tool #when it executes #then does not grant write permission", async () => {
|
||||||
|
const existingFile = createFile("ignored-tool.txt")
|
||||||
|
const sessionID = "ses_ignored_tool"
|
||||||
|
|
||||||
|
await invoke({
|
||||||
|
tool: "edit",
|
||||||
|
sessionID,
|
||||||
|
outputArgs: { filePath: existingFile, oldString: "old", newString: "new" },
|
||||||
|
})
|
||||||
|
|
||||||
|
await expect(
|
||||||
|
invoke({
|
||||||
|
tool: "write",
|
||||||
|
sessionID,
|
||||||
|
outputArgs: { filePath: existingFile, content: "should block" },
|
||||||
|
})
|
||||||
|
).rejects.toThrow(BLOCK_MESSAGE)
|
||||||
|
})
|
||||||
|
|
||||||
test("#given relative read and absolute write #when same session writes #then allows", async () => {
|
test("#given relative read and absolute write #when same session writes #then allows", async () => {
|
||||||
createFile("relative-absolute.txt")
|
createFile("relative-absolute.txt")
|
||||||
const sessionID = "ses_relative_absolute"
|
const sessionID = "ses_relative_absolute"
|
||||||
@ -248,6 +339,45 @@ describe("createWriteExistingFileGuardHook", () => {
|
|||||||
).resolves.toBeDefined()
|
).resolves.toBeDefined()
|
||||||
})
|
})
|
||||||
|
|
||||||
|
test("#given existing file outside session directory #when write executes #then blocks", async () => {
|
||||||
|
const outsideDir = mkdtempSync(join(tmpdir(), "write-existing-file-guard-outside-"))
|
||||||
|
|
||||||
|
try {
|
||||||
|
const outsideFile = join(outsideDir, "outside.txt")
|
||||||
|
writeFileSync(outsideFile, "outside")
|
||||||
|
|
||||||
|
await expect(
|
||||||
|
invoke({
|
||||||
|
tool: "write",
|
||||||
|
outputArgs: { filePath: outsideFile, content: "attempted overwrite" },
|
||||||
|
})
|
||||||
|
).rejects.toThrow(OUTSIDE_SESSION_MESSAGE)
|
||||||
|
} finally {
|
||||||
|
rmSync(outsideDir, { recursive: true, force: true })
|
||||||
|
}
|
||||||
|
})
|
||||||
|
|
||||||
|
test("#given session read permission #when session deleted #then permission is cleaned up", async () => {
|
||||||
|
const existingFile = createFile("session-cleanup.txt")
|
||||||
|
const sessionID = "ses_cleanup"
|
||||||
|
|
||||||
|
await invoke({
|
||||||
|
tool: "read",
|
||||||
|
sessionID,
|
||||||
|
outputArgs: { filePath: existingFile },
|
||||||
|
})
|
||||||
|
|
||||||
|
await emitSessionDeleted(sessionID)
|
||||||
|
|
||||||
|
await expect(
|
||||||
|
invoke({
|
||||||
|
tool: "write",
|
||||||
|
sessionID,
|
||||||
|
outputArgs: { filePath: existingFile, content: "after cleanup" },
|
||||||
|
})
|
||||||
|
).rejects.toThrow(BLOCK_MESSAGE)
|
||||||
|
})
|
||||||
|
|
||||||
test("#given case-different read path #when writing canonical path #then follows platform behavior", async () => {
|
test("#given case-different read path #when writing canonical path #then follows platform behavior", async () => {
|
||||||
const canonicalFile = createFile("CaseFile.txt")
|
const canonicalFile = createFile("CaseFile.txt")
|
||||||
const lowerCasePath = join(tempDir, "casefile.txt")
|
const lowerCasePath = join(tempDir, "casefile.txt")
|
||||||
@ -281,7 +411,11 @@ describe("createWriteExistingFileGuardHook", () => {
|
|||||||
|
|
||||||
try {
|
try {
|
||||||
symlinkSync(targetFile, symlinkPath)
|
symlinkSync(targetFile, symlinkPath)
|
||||||
} catch {
|
} catch (error) {
|
||||||
|
console.warn(
|
||||||
|
"Skipping symlink test: symlinks are not supported or cannot be created in this environment.",
|
||||||
|
error
|
||||||
|
)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -300,6 +434,43 @@ describe("createWriteExistingFileGuardHook", () => {
|
|||||||
).resolves.toBeDefined()
|
).resolves.toBeDefined()
|
||||||
})
|
})
|
||||||
|
|
||||||
|
test("#given session reads beyond path cap #when writing oldest and newest #then only newest is authorized", async () => {
|
||||||
|
const sessionID = "ses_path_cap"
|
||||||
|
const oldestFile = createFile("path-cap/0.txt")
|
||||||
|
let newestFile = oldestFile
|
||||||
|
|
||||||
|
await invoke({
|
||||||
|
tool: "read",
|
||||||
|
sessionID,
|
||||||
|
outputArgs: { filePath: oldestFile },
|
||||||
|
})
|
||||||
|
|
||||||
|
for (let index = 1; index <= MAX_TRACKED_PATHS_PER_SESSION; index += 1) {
|
||||||
|
newestFile = createFile(`path-cap/${index}.txt`)
|
||||||
|
await invoke({
|
||||||
|
tool: "read",
|
||||||
|
sessionID,
|
||||||
|
outputArgs: { filePath: newestFile },
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
|
await expect(
|
||||||
|
invoke({
|
||||||
|
tool: "write",
|
||||||
|
sessionID,
|
||||||
|
outputArgs: { filePath: oldestFile, content: "stale write" },
|
||||||
|
})
|
||||||
|
).rejects.toThrow(BLOCK_MESSAGE)
|
||||||
|
|
||||||
|
await expect(
|
||||||
|
invoke({
|
||||||
|
tool: "write",
|
||||||
|
sessionID,
|
||||||
|
outputArgs: { filePath: newestFile, content: "fresh write" },
|
||||||
|
})
|
||||||
|
).resolves.toBeDefined()
|
||||||
|
})
|
||||||
|
|
||||||
test("#given recently active session #when lru evicts #then keeps recent session permission", async () => {
|
test("#given recently active session #when lru evicts #then keeps recent session permission", async () => {
|
||||||
const existingFile = createFile("lru.txt")
|
const existingFile = createFile("lru.txt")
|
||||||
const hotSession = "ses_hot"
|
const hotSession = "ses_hot"
|
||||||
|
|||||||
@ -383,3 +383,55 @@ type EventInput = { event: { type: string; properties?: Record<string, unknown>
|
|||||||
expect(dispatchCalls[1].event.type).toBe("session.idle")
|
expect(dispatchCalls[1].event.type).toBe("session.idle")
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|
||||||
|
describe("createEventHandler - event forwarding", () => {
|
||||||
|
it("forwards session.deleted to write-existing-file-guard hook", async () => {
|
||||||
|
//#given
|
||||||
|
const forwardedEvents: EventInput[] = []
|
||||||
|
const disconnectedSessions: string[] = []
|
||||||
|
const deletedSessions: string[] = []
|
||||||
|
const eventHandler = createEventHandler({
|
||||||
|
ctx: {} as never,
|
||||||
|
pluginConfig: {} as never,
|
||||||
|
firstMessageVariantGate: {
|
||||||
|
markSessionCreated: () => {},
|
||||||
|
clear: () => {},
|
||||||
|
},
|
||||||
|
managers: {
|
||||||
|
skillMcpManager: {
|
||||||
|
disconnectSession: async (sessionID: string) => {
|
||||||
|
disconnectedSessions.push(sessionID)
|
||||||
|
},
|
||||||
|
},
|
||||||
|
tmuxSessionManager: {
|
||||||
|
onSessionCreated: async () => {},
|
||||||
|
onSessionDeleted: async ({ sessionID }: { sessionID: string }) => {
|
||||||
|
deletedSessions.push(sessionID)
|
||||||
|
},
|
||||||
|
},
|
||||||
|
} as never,
|
||||||
|
hooks: {
|
||||||
|
writeExistingFileGuard: {
|
||||||
|
event: async (input: EventInput) => {
|
||||||
|
forwardedEvents.push(input)
|
||||||
|
},
|
||||||
|
},
|
||||||
|
} as never,
|
||||||
|
})
|
||||||
|
const sessionID = "ses_forward_delete_event"
|
||||||
|
|
||||||
|
//#when
|
||||||
|
await eventHandler({
|
||||||
|
event: {
|
||||||
|
type: "session.deleted",
|
||||||
|
properties: { info: { id: sessionID } },
|
||||||
|
},
|
||||||
|
})
|
||||||
|
|
||||||
|
//#then
|
||||||
|
expect(forwardedEvents.length).toBe(1)
|
||||||
|
expect(forwardedEvents[0]?.event.type).toBe("session.deleted")
|
||||||
|
expect(disconnectedSessions).toEqual([sessionID])
|
||||||
|
expect(deletedSessions).toEqual([sessionID])
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|||||||
@ -48,6 +48,7 @@ export function createEventHandler(args: {
|
|||||||
await Promise.resolve(hooks.ralphLoop?.event?.(input))
|
await Promise.resolve(hooks.ralphLoop?.event?.(input))
|
||||||
await Promise.resolve(hooks.stopContinuationGuard?.event?.(input))
|
await Promise.resolve(hooks.stopContinuationGuard?.event?.(input))
|
||||||
await Promise.resolve(hooks.compactionTodoPreserver?.event?.(input))
|
await Promise.resolve(hooks.compactionTodoPreserver?.event?.(input))
|
||||||
|
await Promise.resolve(hooks.writeExistingFileGuard?.event?.(input))
|
||||||
await Promise.resolve(hooks.atlasHook?.handler?.(input))
|
await Promise.resolve(hooks.atlasHook?.handler?.(input))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user