mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-12 23:01:21 +00:00
Hook Middleware & Security Boundary and test done
This commit is contained in:
parent
5f225d946f
commit
04965a7cf0
12 changed files with 326 additions and 8 deletions
62
scripts/pre-hook.spec.ts
Normal file
62
scripts/pre-hook.spec.ts
Normal file
|
|
@ -0,0 +1,62 @@
|
|||
import { describe, it, expect, beforeEach } from "vitest"
|
||||
import { PreHook } from "../pre-hook"
|
||||
|
||||
describe("PreHook", () => {
|
||||
let activeIntentId: string | null
|
||||
let preHook: PreHook
|
||||
|
||||
beforeEach(() => {
|
||||
activeIntentId = null
|
||||
preHook = new PreHook({
|
||||
cwd: process.cwd(),
|
||||
getActiveIntentId: () => activeIntentId,
|
||||
setActiveIntentId: (id) => {
|
||||
activeIntentId = id
|
||||
},
|
||||
requireIntentForDestructiveOnly: true,
|
||||
})
|
||||
})
|
||||
|
||||
it("blocks destructive write without active intent", async () => {
|
||||
activeIntentId = null
|
||||
const res = await preHook.intercept("write_to_file", {
|
||||
path: "src/api/weather.ts",
|
||||
content: "// test",
|
||||
})
|
||||
expect(res.blocked).toBe(true)
|
||||
expect(res.error).toEqual(expect.stringMatching(/select an active intent/i))
|
||||
})
|
||||
|
||||
it("blocks write outside owned scope (scope violation)", async () => {
|
||||
activeIntentId = "INT-001"
|
||||
const res = await preHook.intercept("write_to_file", {
|
||||
path: "src/db/db.ts",
|
||||
content: "// should be blocked",
|
||||
})
|
||||
expect(res.blocked).toBe(true)
|
||||
expect(res.error).toEqual(expect.stringMatching(/scope violation/i))
|
||||
})
|
||||
|
||||
it("recovery loop: retry after select_active_intent succeeds", async () => {
|
||||
// initial attempt without intent
|
||||
activeIntentId = null
|
||||
const attempt1 = await preHook.intercept("write_to_file", {
|
||||
path: "src/api/weather.ts",
|
||||
content: "// first",
|
||||
})
|
||||
expect(attempt1.blocked).toBe(true)
|
||||
|
||||
// select active intent (handshake)
|
||||
const handshake = await preHook.intercept("select_active_intent", { intent_id: "INT-001" })
|
||||
expect(handshake.blocked).toBe(false)
|
||||
expect(handshake.injectResult).toEqual(expect.stringContaining("<intent_context>"))
|
||||
expect(activeIntentId).toBe("INT-001")
|
||||
|
||||
// retry write (in-owned-scope path)
|
||||
const attempt2 = await preHook.intercept("write_to_file", {
|
||||
path: "src/api/weather.ts",
|
||||
content: "// second",
|
||||
})
|
||||
expect(attempt2.blocked).toBe(false)
|
||||
})
|
||||
})
|
||||
72
scripts/pre-hook.test.ts
Normal file
72
scripts/pre-hook.test.ts
Normal file
|
|
@ -0,0 +1,72 @@
|
|||
import test from "node:test"
|
||||
import assert from "node:assert/strict"
|
||||
import { PreHook } from "../src/hooks/pre-hook"
|
||||
|
||||
const cwd = process.cwd()
|
||||
|
||||
test("blocks destructive write without active intent", async () => {
|
||||
let activeIntentId: string | null = null
|
||||
const preHook = new PreHook({
|
||||
cwd,
|
||||
getActiveIntentId: () => activeIntentId,
|
||||
setActiveIntentId: (id) => {
|
||||
activeIntentId = id
|
||||
},
|
||||
requireIntentForDestructiveOnly: true,
|
||||
})
|
||||
|
||||
const res = await preHook.intercept("write_to_file", {
|
||||
path: "src/api/weather.ts",
|
||||
content: "// test",
|
||||
})
|
||||
assert.equal(res.blocked, true)
|
||||
assert.match(String(res.error), /select an active intent/i)
|
||||
})
|
||||
|
||||
test("blocks write outside owned scope (scope violation)", async () => {
|
||||
let activeIntentId: string | null = "INT-001"
|
||||
const preHook = new PreHook({
|
||||
cwd,
|
||||
getActiveIntentId: () => activeIntentId,
|
||||
setActiveIntentId: (id) => {
|
||||
activeIntentId = id
|
||||
},
|
||||
requireIntentForDestructiveOnly: true,
|
||||
})
|
||||
|
||||
const res = await preHook.intercept("write_to_file", {
|
||||
path: "src/db/db.ts",
|
||||
content: "// should be blocked",
|
||||
})
|
||||
assert.equal(res.blocked, true)
|
||||
assert.match(String(res.error), /scope violation/i)
|
||||
})
|
||||
|
||||
test("recovery loop: retry after select_active_intent succeeds", async () => {
|
||||
let activeIntentId: string | null = null
|
||||
const preHook = new PreHook({
|
||||
cwd,
|
||||
getActiveIntentId: () => activeIntentId,
|
||||
setActiveIntentId: (id) => {
|
||||
activeIntentId = id
|
||||
},
|
||||
requireIntentForDestructiveOnly: true,
|
||||
})
|
||||
|
||||
const attempt1 = await preHook.intercept("write_to_file", {
|
||||
path: "src/api/weather.ts",
|
||||
content: "// first",
|
||||
})
|
||||
assert.equal(attempt1.blocked, true)
|
||||
|
||||
const handshake = await preHook.intercept("select_active_intent", { intent_id: "INT-001" })
|
||||
assert.equal(handshake.blocked, false)
|
||||
assert.ok(typeof handshake.injectResult === "string" && handshake.injectResult.includes("<intent_context>"))
|
||||
assert.equal(activeIntentId, "INT-001")
|
||||
|
||||
const attempt2 = await preHook.intercept("write_to_file", {
|
||||
path: "src/api/weather.ts",
|
||||
content: "// second",
|
||||
})
|
||||
assert.equal(attempt2.blocked, false)
|
||||
})
|
||||
|
|
@ -40,6 +40,7 @@ import { codebaseSearchTool } from "../tools/CodebaseSearchTool"
|
|||
|
||||
import { formatResponse } from "../prompts/responses"
|
||||
import { sanitizeToolUseId } from "../../utils/tool-id"
|
||||
import { HookMiddleware, PreHook } from "../../hooks"
|
||||
|
||||
/**
|
||||
* Processes and presents assistant message content to the user interface.
|
||||
|
|
@ -676,6 +677,9 @@ export async function presentAssistantMessage(cline: Task) {
|
|||
}
|
||||
|
||||
switch (block.name) {
|
||||
case "select_active_intent":
|
||||
// Handled entirely by pre-hook (injectResult pushed above)
|
||||
break
|
||||
case "write_to_file":
|
||||
await checkpointSaveAndMark(cline)
|
||||
await writeToFileTool.handle(cline, block as ToolUse<"write_to_file">, {
|
||||
|
|
|
|||
|
|
@ -30,6 +30,27 @@ export const formatResponse = {
|
|||
error,
|
||||
}),
|
||||
|
||||
/** Standardized JSON for autonomous recovery when user rejects a destructive tool. */
|
||||
toolErrorUserRejected: (toolName?: string) =>
|
||||
JSON.stringify({
|
||||
status: "error",
|
||||
type: "user_rejected",
|
||||
message: "The user rejected this operation.",
|
||||
tool: toolName,
|
||||
suggestion: "Do not retry the same operation; try a different approach or ask the user for permission.",
|
||||
}),
|
||||
|
||||
/** Standardized JSON for scope violation so the LLM can request scope expansion. */
|
||||
toolErrorScopeViolation: (intentId: string, filename: string) =>
|
||||
JSON.stringify({
|
||||
status: "error",
|
||||
type: "scope_violation",
|
||||
message: `Scope Violation: ${intentId} is not authorized to edit [${filename}]. Request scope expansion.`,
|
||||
intent_id: intentId,
|
||||
path: filename,
|
||||
suggestion: "Request scope expansion in .orchestration/active_intents.yaml or choose another intent.",
|
||||
}),
|
||||
|
||||
rooIgnoreError: (path: string) =>
|
||||
JSON.stringify({
|
||||
status: "error",
|
||||
|
|
|
|||
|
|
@ -266,6 +266,8 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
|
|||
|
||||
providerRef: WeakRef<ClineProvider>
|
||||
private readonly globalStoragePath: string
|
||||
/** Active intent ID set by select_active_intent (per task/session). Used by Hook Engine for scope enforcement. */
|
||||
private _activeIntentId: string | null = null
|
||||
abort: boolean = false
|
||||
currentRequestAbortController?: AbortController
|
||||
skipPrevResponseIdOnce: boolean = false
|
||||
|
|
@ -4670,6 +4672,15 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
|
|||
return this.workspacePath
|
||||
}
|
||||
|
||||
/** Active intent ID for Hook Engine (select_active_intent / scope enforcement). */
|
||||
public getActiveIntentId(): string | null {
|
||||
return this._activeIntentId
|
||||
}
|
||||
|
||||
public setActiveIntentId(id: string | null): void {
|
||||
this._activeIntentId = id
|
||||
}
|
||||
|
||||
/**
|
||||
* Provides convenient access to high-level message operations.
|
||||
* Uses lazy initialization - the MessageManager is only created when first accessed.
|
||||
|
|
|
|||
|
|
@ -3,6 +3,13 @@ import type { ToolName } from "@roo-code/types"
|
|||
import { Task } from "../task/Task"
|
||||
import type { ToolUse, HandleError, PushToolResult, AskApproval, NativeToolArgs } from "../../shared/tools"
|
||||
|
||||
/** Params passed to onWriteToFileSuccess after a successful write_to_file (for Hook Engine post-hook). */
|
||||
export interface WriteToFileSuccessParams {
|
||||
path: string
|
||||
content: string
|
||||
mutation_class?: string
|
||||
}
|
||||
|
||||
/**
|
||||
* Callbacks passed to tool execution
|
||||
*/
|
||||
|
|
@ -11,6 +18,8 @@ export interface ToolCallbacks {
|
|||
handleError: HandleError
|
||||
pushToolResult: PushToolResult
|
||||
toolCallId?: string
|
||||
/** Called after successful write_to_file for Hook Engine agent_trace / post-hook. */
|
||||
onWriteToFileSuccess?: (params: WriteToFileSuccessParams) => void | Promise<void>
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -179,6 +179,13 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
|
|||
|
||||
pushToolResult(message)
|
||||
|
||||
const mutationClass = (params as Record<string, unknown>).mutation_class as string | undefined
|
||||
await callbacks.onWriteToFileSuccess?.({
|
||||
path: relPath,
|
||||
content: newContent,
|
||||
mutation_class: mutationClass,
|
||||
})
|
||||
|
||||
await task.diffViewProvider.reset()
|
||||
this.resetPartialState()
|
||||
|
||||
|
|
|
|||
|
|
@ -7,6 +7,7 @@ export * from "./types"
|
|||
export * from "./content-hash"
|
||||
export * from "./context-loader"
|
||||
export * from "./scope"
|
||||
export * from "./intent-ignore"
|
||||
export * from "./pre-hook"
|
||||
export * from "./post-hook"
|
||||
export * from "./middleware"
|
||||
|
|
|
|||
54
src/hooks/intent-ignore.ts
Normal file
54
src/hooks/intent-ignore.ts
Normal file
|
|
@ -0,0 +1,54 @@
|
|||
import fs from "fs/promises"
|
||||
import path from "path"
|
||||
|
||||
import { pathMatchesAnyPattern } from "./scope"
|
||||
|
||||
const DEFAULT_INTENT_IGNORE_NAME = ".intentignore"
|
||||
const ORCHESTRATION_DIR = ".orchestration"
|
||||
const INTENT_PREFIX = "intent:"
|
||||
|
||||
export interface IntentIgnoreResult {
|
||||
pathPatterns: string[]
|
||||
excludedIntentIds: string[]
|
||||
}
|
||||
|
||||
/**
|
||||
* Load .intentignore-style file: path patterns (one per line) and optional
|
||||
* "intent:ID" lines to exclude specific intents from receiving changes.
|
||||
* Convention: lines starting with "intent:" are intent IDs to exclude; all other
|
||||
* non-empty, non-comment lines are path glob patterns that no write may touch.
|
||||
*
|
||||
* @param cwd - Workspace root
|
||||
* @param intentIgnorePath - Optional path relative to cwd (e.g. ".orchestration/.intentignore")
|
||||
*/
|
||||
export async function loadIntentIgnore(cwd: string, intentIgnorePath?: string): Promise<IntentIgnoreResult> {
|
||||
const filePath = intentIgnorePath
|
||||
? path.resolve(cwd, intentIgnorePath)
|
||||
: path.join(cwd, ORCHESTRATION_DIR, DEFAULT_INTENT_IGNORE_NAME)
|
||||
try {
|
||||
const raw = await fs.readFile(filePath, "utf-8")
|
||||
const pathPatterns: string[] = []
|
||||
const excludedIntentIds: string[] = []
|
||||
for (const line of raw.split("\n")) {
|
||||
const trimmed = line.trim()
|
||||
if (!trimmed || trimmed.startsWith("#")) continue
|
||||
if (trimmed.startsWith(INTENT_PREFIX)) {
|
||||
excludedIntentIds.push(trimmed.slice(INTENT_PREFIX.length).trim())
|
||||
} else {
|
||||
pathPatterns.push(trimmed)
|
||||
}
|
||||
}
|
||||
return { pathPatterns, excludedIntentIds }
|
||||
} catch {
|
||||
return { pathPatterns: [], excludedIntentIds: [] }
|
||||
}
|
||||
}
|
||||
|
||||
export function isPathIgnored(relativePath: string, pathPatterns: string[]): boolean {
|
||||
return pathMatchesAnyPattern(relativePath, pathPatterns)
|
||||
}
|
||||
|
||||
export function isIntentExcluded(intentId: string | null, excludedIntentIds: string[]): boolean {
|
||||
if (!intentId) return false
|
||||
return excludedIntentIds.some((id) => id === intentId)
|
||||
}
|
||||
|
|
@ -27,11 +27,19 @@ export interface PreHookOptions {
|
|||
/**
|
||||
* Pre-Hook: intercepts tool execution to enforce intent context and scope.
|
||||
* - select_active_intent: load context, return XML, set active intent.
|
||||
* - Destructive tools: require active intent; optional HITL; scope check for write_to_file.
|
||||
* - Destructive tools: require active intent; .intentignore exclusion; scope check for write_to_file; optional UI-blocking approval.
|
||||
*/
|
||||
export class PreHook {
|
||||
private intentIgnoreCache: IntentIgnoreResult | null = null
|
||||
|
||||
constructor(private options: PreHookOptions) {}
|
||||
|
||||
private async getIntentIgnore(): Promise<IntentIgnoreResult> {
|
||||
if (this.intentIgnoreCache) return this.intentIgnoreCache
|
||||
this.intentIgnoreCache = await loadIntentIgnore(this.options.cwd, this.options.intentIgnorePath)
|
||||
return this.intentIgnoreCache
|
||||
}
|
||||
|
||||
async intercept(toolName: string, params: Record<string, unknown>): Promise<HookResult> {
|
||||
const { cwd, getActiveIntentId, setActiveIntentId } = this.options
|
||||
|
||||
|
|
@ -54,9 +62,9 @@ export class PreHook {
|
|||
|
||||
const isDestructive = (DESTRUCTIVE_TOOLS as readonly string[]).includes(toolName)
|
||||
const requireIntent = this.options.requireIntentForDestructiveOnly ? isDestructive : true
|
||||
const activeId = getActiveIntentId()
|
||||
|
||||
if (requireIntent) {
|
||||
const activeId = getActiveIntentId()
|
||||
if (!activeId) {
|
||||
return {
|
||||
blocked: true,
|
||||
|
|
@ -64,27 +72,67 @@ export class PreHook {
|
|||
}
|
||||
}
|
||||
|
||||
// Scope enforcement for write_to_file
|
||||
if (toolName === "write_to_file" && params.path) {
|
||||
const relPath = String(params.path)
|
||||
// Path traversal: block paths that escape workspace (e.g. .. or absolute)
|
||||
const ignore = await this.getIntentIgnore()
|
||||
if (isIntentExcluded(activeId, ignore.excludedIntentIds)) {
|
||||
return {
|
||||
blocked: true,
|
||||
error: formatResponse.toolError(
|
||||
`Intent ${activeId} is listed in .intentignore and cannot be modified. Choose another intent or ask the user to update .intentignore.`,
|
||||
),
|
||||
}
|
||||
}
|
||||
|
||||
// Scope and .intentignore path checks for file-writing tools
|
||||
const filePathParam =
|
||||
toolName === "write_to_file"
|
||||
? params.path
|
||||
: [
|
||||
"edit",
|
||||
"search_and_replace",
|
||||
"search_replace",
|
||||
"edit_file",
|
||||
"apply_patch",
|
||||
"apply_diff",
|
||||
].includes(toolName)
|
||||
? (params.file_path ?? params.path)
|
||||
: undefined
|
||||
if (filePathParam) {
|
||||
const relPath = String(filePathParam)
|
||||
if (isPathTraversal(relPath, cwd)) {
|
||||
return {
|
||||
blocked: true,
|
||||
error: `Path traversal not allowed: "${relPath}" would escape the workspace. Use a path relative to the workspace only.`,
|
||||
}
|
||||
}
|
||||
if (isPathIgnored(relPath, ignore.pathPatterns)) {
|
||||
return {
|
||||
blocked: true,
|
||||
error: formatResponse.toolError(
|
||||
`Path "${relPath}" is excluded by .intentignore. You are not authorized to edit it.`,
|
||||
),
|
||||
}
|
||||
}
|
||||
const context = await loadIntentContext(cwd, activeId)
|
||||
if (context && context.owned_scope.length > 0 && !pathInScope(relPath, context.owned_scope, cwd)) {
|
||||
return {
|
||||
blocked: true,
|
||||
error: `Scope Violation: ${activeId} is not authorized to edit "${relPath}". Request scope expansion in active_intents.yaml or choose another intent.`,
|
||||
error: formatResponse.toolErrorScopeViolation(activeId, relPath),
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Optional: HITL for destructive tools (wire via askApproval in host / extension)
|
||||
// UI-blocking authorization for destructive tools (e.g. showWarningMessage Approve/Reject)
|
||||
if (isDestructive && this.options.confirmDestructive) {
|
||||
const approved = await this.options.confirmDestructive(toolName, params)
|
||||
if (!approved) {
|
||||
return {
|
||||
blocked: true,
|
||||
error: formatResponse.toolErrorUserRejected(toolName),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return { blocked: false }
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -37,3 +37,24 @@ function simpleGlobMatch(path: string, pattern: string): boolean {
|
|||
)
|
||||
return re.test(path)
|
||||
}
|
||||
|
||||
/**
|
||||
* Check if a relative path matches any of the given glob-like patterns.
|
||||
* Used by .intentignore to exclude paths from edits.
|
||||
*/
|
||||
export function pathMatchesAnyPattern(relativePath: string, patterns: string[]): boolean {
|
||||
if (!patterns || patterns.length === 0) return false
|
||||
const normalized = path.normalize(relativePath).replace(/\\/g, "/")
|
||||
for (const pattern of patterns) {
|
||||
const p = path.normalize(pattern).replace(/\\/g, "/")
|
||||
if (p.endsWith("/**")) {
|
||||
const prefix = p.slice(0, -3)
|
||||
if (normalized === prefix || normalized.startsWith(prefix + "/")) return true
|
||||
} else if (p.includes("*")) {
|
||||
if (simpleGlobMatch(normalized, p)) return true
|
||||
} else {
|
||||
if (normalized === p || normalized.endsWith("/" + p)) return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
|
|
|||
|
|
@ -64,6 +64,14 @@ export interface AgentTraceEntry {
|
|||
/** Safe = read-only; Destructive = write, delete, execute */
|
||||
export type CommandClass = "safe" | "destructive"
|
||||
|
||||
/**
|
||||
* Classify a tool name as Safe (read) or Destructive (write, delete, execute).
|
||||
* Used by the Hook Engine for authorization and UI-blocking.
|
||||
*/
|
||||
export function classifyCommand(toolName: string): CommandClass {
|
||||
return (DESTRUCTIVE_TOOLS as readonly string[]).includes(toolName) ? "destructive" : "safe"
|
||||
}
|
||||
|
||||
export const DESTRUCTIVE_TOOLS = [
|
||||
"write_to_file",
|
||||
"apply_diff",
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue