diff --git a/.gitignore b/.gitignore index 5590afdf..0a6c49f8 100644 --- a/.gitignore +++ b/.gitignore @@ -44,6 +44,8 @@ e2e/.openfox/ e2e-playwright/.openfox/ e2e/.openfox-test/ e2e/**/.openfox-test/ +.openfox/pr-description.md +.openfox/permissions.json # Logs *.log diff --git a/e2e/path-security.test.ts b/e2e/path-security.test.ts index 603483e9..d6b5159c 100644 --- a/e2e/path-security.test.ts +++ b/e2e/path-security.test.ts @@ -8,7 +8,7 @@ * The path security system: * 1. Detects operations on paths outside workdir or sensitive files * 2. Emits chat.path_confirmation event to client - * 3. Waits for user approval via path.confirm message + * 3. Waits for user approval via REST API * 4. Proceeds or aborts based on user response */ @@ -239,9 +239,10 @@ describe('Path Security', () => { if (confirmationEvent) { const payload = confirmationEvent.payload as PathConfirmationPayload + const session = client.getSession()! // Approve - await client.answerPathConfirmation(payload.callId, true) + await answerPathConfirmation(server.url, session.id, payload.callId, true) await client.waitFor('chat.done').catch(() => null) client.clearEvents() diff --git a/e2e/permission-rules.test.ts b/e2e/permission-rules.test.ts new file mode 100644 index 00000000..04df5653 --- /dev/null +++ b/e2e/permission-rules.test.ts @@ -0,0 +1,183 @@ +/** + * Permission Rules E2E Tests + * + * Tests server-side permission rules (ALLOW/DENY/ASK) through the full + * server stack: config file → rule loading → tool execution → confirmation. + * + * Flows covered: + * 1. DENY rule blocks a run_command tool call with rule_denied reason + * 2. ASK rule emits a path_confirmation event that can be approved + * 3. Approving with alwaysAllow=true promotes to a session ALLOW rule + * so a second identical call does not re-prompt + */ + +import { describe, it, expect, beforeAll, afterAll, beforeEach, afterEach } from 'vitest' +import { writeFile, mkdir } from 'node:fs/promises' +import { join } from 'node:path' +import { + createTestClient, + createTestProject, + createTestServer, + createProject, + createSession, + setSessionMode, + answerPathConfirmation, + type TestClient, + type TestProject, + type TestServerHandle, +} from './utils/index.js' + +interface PathConfirmationPayload { + callId: string + tool: string + paths: string[] + workdir: string + reason: + 'outside_workdir' | 'sensitive_file' | 'both' | 'dangerous_command' | 'git_no_verify' | 'rule_denied' | 'rule_ask' +} + +async function writeProjectPermissions( + workdir: string, + rules: Array<{ effect: string; tool: string; pattern?: string }>, +): Promise { + const dir = join(workdir, '.openfox') + await mkdir(dir, { recursive: true }) + const config = { version: 1, rules } + await writeFile(join(dir, 'permissions.json'), JSON.stringify(config, null, 2) + '\n', 'utf-8') +} + +describe('Permission Rules', () => { + let server: TestServerHandle + let client: TestClient + let testDir: TestProject + + beforeAll(async () => { + server = await createTestServer() + }) + + afterAll(async () => { + await server.close() + }) + + beforeEach(async () => { + client = await createTestClient({ url: server.wsUrl }) + testDir = await createTestProject({ template: 'typescript' }) + }) + + afterEach(async () => { + await client.close() + await testDir.cleanup() + }) + + async function setupSession(): Promise { + const restProject = await createProject(server.url, { name: 'Permission Rules Test', workdir: testDir.path }) + const restSession = await createSession(server.url, { projectId: restProject.id }) + await client.send('session.load', { sessionId: restSession.id }) + await setSessionMode(server.url, restSession.id, 'builder', server.wsUrl) + return restSession.id + } + + it('DENY rule on run_command blocks execution with rule_denied reason', async () => { + await writeProjectPermissions(testDir.path, [{ effect: 'DENY', tool: 'run_command', pattern: 'rm -rf *' }]) + await setupSession() + + client.clearEvents() + await client.send('chat.send', { + content: 'Run rm -rf root to delete everything', + }) + + const response = await client.waitForChatDone() + + // The tool call should have been blocked by the DENY rule + const runCommandCalls = response.toolCalls.filter((tc) => tc.tool === 'run_command') + expect(runCommandCalls.length).toBeGreaterThan(0) + + const blockedCall = runCommandCalls[0]! + expect(blockedCall.result).toBeDefined() + expect(blockedCall.result!.success).toBe(false) + expect(blockedCall.result!.error).toContain('blocked by a permission rule') + + // No path_confirmation event should be emitted for DENY (it throws directly) + const confirmationEvents = client.allEvents().filter((e) => e.type === 'chat.path_confirmation') + expect(confirmationEvents.length).toBe(0) + }) + + it('ASK rule on run_command emits path_confirmation with rule_ask reason', async () => { + await writeProjectPermissions(testDir.path, [ + { effect: 'ASK', tool: 'run_command', pattern: 'terragrunt destroy *' }, + ]) + const sessionId = await setupSession() + + client.clearEvents() + await client.send('chat.send', { + content: 'Run terragrunt destroy on the test environment', + }) + + // Wait for the path_confirmation event + const confirmationEvent = await client.waitFor('chat.path_confirmation', undefined, 5000).catch(() => null) + + expect(confirmationEvent).not.toBeNull() + const payload = confirmationEvent!.payload as PathConfirmationPayload + expect(payload.reason).toBe('rule_ask') + expect(payload.tool).toBe('run_command') + expect(payload.callId).toBeDefined() + + // Deny the confirmation + await answerPathConfirmation(server.url, sessionId, payload.callId, false) + + await client.waitForChatDone().catch(() => null) + + // The tool result should reflect the denial + const response = client.allEvents() + const toolResults = response.filter((e) => e.type === 'chat.tool_result') + const deniedResult = toolResults.find((e) => { + const payload = e.payload as { result?: { success?: boolean; error?: string } } + return payload.result?.success === false && payload.result?.error?.includes('permission rule') + }) + expect(deniedResult).toBeDefined() + }) + + it('ASK rule → approve with alwaysAllow → second call does not re-prompt', async () => { + await writeProjectPermissions(testDir.path, [ + { effect: 'ASK', tool: 'run_command', pattern: 'terragrunt destroy *' }, + ]) + const sessionId = await setupSession() + + // --- First call: triggers ASK confirmation --- + client.clearEvents() + await client.send('chat.send', { + content: 'Run terragrunt destroy on the test environment', + }) + + const confirmationEvent1 = await client.waitFor('chat.path_confirmation', undefined, 5000).catch(() => null) + expect(confirmationEvent1).not.toBeNull() + const payload1 = confirmationEvent1!.payload as PathConfirmationPayload + expect(payload1.reason).toBe('rule_ask') + + // Approve with alwaysAllow=true (promotes ASK to session ALLOW) + await answerPathConfirmation(server.url, sessionId, payload1.callId, true, true) + await client.waitForChatDone().catch(() => null) + + // --- Second call: should NOT re-prompt (session ALLOW rule active) --- + client.clearEvents() + await client.send('chat.send', { + content: 'Run terragrunt destroy on the test environment again', + }) + + const response2 = await client.waitForChatDone() + + // No path_confirmation event should be emitted on the second call + const confirmationEvents2 = client.allEvents().filter((e) => e.type === 'chat.path_confirmation') + expect(confirmationEvents2.length).toBe(0) + + // The tool should have been called (not blocked by ASK/DENY) + const runCommandCalls2 = response2.toolCalls.filter((tc) => tc.tool === 'run_command') + expect(runCommandCalls2.length).toBeGreaterThan(0) + + // The tool should NOT have been blocked by a permission rule + const blockedCall = runCommandCalls2.find( + (tc) => tc.result?.success === false && tc.result?.error?.includes('permission rule'), + ) + expect(blockedCall).toBeUndefined() + }) +}) diff --git a/e2e/utils/rest-client.ts b/e2e/utils/rest-client.ts index 7cae1f95..568ff649 100644 --- a/e2e/utils/rest-client.ts +++ b/e2e/utils/rest-client.ts @@ -256,11 +256,16 @@ export async function answerPathConfirmation( sessionId: string, callId: string, approved: boolean, + alwaysAllow?: boolean, ): Promise<{ success: boolean }> { + const body: Record = { callId, approved } + if (alwaysAllow !== undefined) { + body['alwaysAllow'] = alwaysAllow + } const response = await fetch(`${baseUrl}/api/sessions/${sessionId}/confirm-path`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ callId, approved }), + body: JSON.stringify(body), }) if (!response.ok) { diff --git a/e2e/utils/ws-client.ts b/e2e/utils/ws-client.ts index 952c297a..7686740f 100644 --- a/e2e/utils/ws-client.ts +++ b/e2e/utils/ws-client.ts @@ -6,7 +6,6 @@ import { WebSocket } from 'ws' import type { - ClientMessage, ServerMessage, ServerMessageType, ChatDonePayload, @@ -78,9 +77,6 @@ export interface TestClient { /** Check if connected */ isConnected(): boolean - - /** Answer a pending path confirmation (for e2e tests) */ - answerPathConfirmation(callId: string, approved: boolean): Promise } const CI_MULTIPLIER = process.env['CI'] === 'true' ? 10 : 1 @@ -703,16 +699,5 @@ export async function createTestClient(options: TestClientOptions = {}): Promise isConnected(): boolean { return connected && ws.readyState === WebSocket.OPEN }, - - async answerPathConfirmation(callId: string, approved: boolean): Promise { - const id = crypto.randomUUID() - const message: ClientMessage = { - id, - type: 'path.confirm', - payload: { callId, approved }, - } - ws.send(JSON.stringify(message)) - // No artificial delay - server processes synchronously - }, } } diff --git a/src/server/chat/agent-loop.test.ts b/src/server/chat/agent-loop.test.ts index 6a81a5c2..2dc68213 100644 --- a/src/server/chat/agent-loop.test.ts +++ b/src/server/chat/agent-loop.test.ts @@ -1034,3 +1034,194 @@ describe('runTopLevelAgentLoop queue draining', () => { expect(queuedInHistory).toHaveLength(0) }) }) + +// ============================================================================ +// Permission rules merge wiring: disk rules + session-allowed rules +// ============================================================================ + +describe('runTopLevelAgentLoop permission rules merge', () => { + let mockEventStore: EventStore + let mockSessionManager: any + let mockLLMClient: any + let mockTurnMetrics: TurnMetrics + let mockToolRegistry: ToolRegistry + let assembleRequestMock: ReturnType + let mockAppend: ReturnType + let capturedBatchContext: any + + beforeEach(() => { + vi.clearAllMocks() + + mockEventStore = { + append: vi.fn(), + getEvents: vi.fn().mockReturnValue([]), + getLatestSeq: vi.fn().mockReturnValue(0), + cleanupOldEvents: vi.fn().mockReturnValue(0), + } as unknown as EventStore + ;(getEventStore as any).mockReturnValue(mockEventStore) + + mockLLMClient = { getModel: vi.fn().mockReturnValue('test-model') } + + mockTurnMetrics = { + addToolTime: vi.fn(), + addLLMCall: vi.fn(), + buildStats: vi.fn().mockReturnValue({}), + } as unknown as TurnMetrics + + assembleRequestMock = vi.fn().mockReturnValue({ systemPrompt: 'test', messages: [] }) + ;(getAllInstructions as any).mockResolvedValue({ content: 'test', files: [] }) + ;(getEnabledSkillMetadata as any).mockResolvedValue([]) + + mockToolRegistry = { + tools: [], + definitions: [], + execute: vi.fn().mockResolvedValue({ success: true, output: 'ok', durationMs: 0, truncated: false }), + } as unknown as ToolRegistry + + // Iteration 1: tool batch (captures batchContext), iteration 2: no tools (terminates) + ;(consumeStreamGenerator as any) + .mockResolvedValueOnce({ + content: '', + toolCalls: [{ id: 'call-1', name: 'read_file', arguments: { path: 'a.ts' } }], + segments: [], + usage: { promptTokens: 10, completionTokens: 5 }, + timing: { ttft: 0.1, completionTime: 0.5, tps: 10, prefillTps: 100 }, + aborted: false, + finishReason: 'stop', + modelParams: {}, + }) + .mockResolvedValue({ + content: '', + toolCalls: [], + segments: [], + usage: { promptTokens: 10, completionTokens: 5 }, + timing: { ttft: 0.1, completionTime: 0.5, tps: 10, prefillTps: 100 }, + aborted: false, + finishReason: 'stop', + modelParams: {}, + }) + + mockSessionManager = { + requireSession: vi.fn().mockReturnValue({ + workdir: '/test', + projectId: 'test-project', + executionState: null, + criteria: [], + isRunning: false, + }), + getEffectiveWorkdir: vi.fn().mockReturnValue('/test'), + getProjectWorkdir: vi.fn().mockReturnValue('/test'), + getContextState: vi.fn().mockReturnValue({ + currentTokens: 0, + maxTokens: 200000, + compactionCount: 0, + dangerZone: false, + canCompact: false, + dynamicContextChanged: false, + }), + getCurrentModelContext: vi.fn().mockReturnValue(200000), + getCurrentModelSettings: vi.fn().mockReturnValue({}), + getModelCompactionThreshold: vi.fn().mockReturnValue(undefined), + setCurrentContextSize: vi.fn(), + getDynamicContextChanged: vi.fn().mockReturnValue(false), + setDynamicContextChanged: vi.fn(), + getCachedPrompt: vi.fn().mockReturnValue(undefined), + setCachedPrompt: vi.fn(), + getLspManager: vi.fn(), + drainAsapMessages: vi.fn().mockReturnValue([]), + getQueueState: vi.fn().mockReturnValue([]), + getCurrentWindowMessages: vi.fn().mockReturnValue([]), + updateMessage: vi.fn(), + } as any + + capturedBatchContext = undefined + }) + + function makeConfig(overrides?: Partial): TopLevelLoopConfig { + mockAppend = vi.fn() + return { + mode: 'planner', + append: mockAppend as any, + sessionManager: mockSessionManager, + sessionId: 'test-session', + llmClient: mockLLMClient, + statsIdentity: { providerId: 'test', providerName: 'Test', backend: 'unknown' as const, model: 'test-model' }, + assembleRequest: assembleRequestMock as any, + getToolRegistry: () => mockToolRegistry as any, + getConversationMessages: vi.fn().mockResolvedValue([]), + onMessage: vi.fn(), + ...overrides, + } + } + + it('merges disk rules and session-allowed rules into batchContext.permissionRules', async () => { + // Stub the dynamic imports inside agent-loop + vi.doMock('../permissions/registry.js', () => ({ + loadMergedRules: vi.fn().mockResolvedValue([{ effect: 'DENY', tool: 'run_command', pattern: 'rm -rf *' }]), + })) + vi.doMock('../tools/path-security.js', () => ({ + getSessionAllowedRules: vi.fn().mockReturnValue([{ effect: 'ALLOW', tool: 'read_file', pattern: '/tmp/**' }]), + clearAllowedPaths: vi.fn(), + addSessionAllowedRule: vi.fn(), + addAllowedPath: vi.fn(), + addAllowedPaths: vi.fn(), + isPathAllowed: vi.fn().mockReturnValue(false), + requestPathAccess: vi.fn(), + PathAccessDeniedError: class PathAccessDeniedError extends Error {}, + AskUserInterrupt: class AskUserInterrupt extends Error {}, + })) + + // Capture batchContext by spying on executeTools + const executeToolsSpy = vi + .spyOn(await import('./execute-tools.js'), 'executeTools') + .mockImplementation(async (_msgId: string, _calls: any, ctx: any) => { + capturedBatchContext = ctx + return { toolMessages: [], criteriaChanged: false } + }) + + await runTopLevelAgentLoop(makeConfig(), mockTurnMetrics) + + expect(capturedBatchContext).toBeDefined() + expect(capturedBatchContext.permissionRules).toEqual([ + { effect: 'DENY', tool: 'run_command', pattern: 'rm -rf *' }, + { effect: 'ALLOW', tool: 'read_file', pattern: '/tmp/**' }, + ]) + + executeToolsSpy.mockRestore() + vi.doUnmock('../permissions/registry.js') + vi.doUnmock('../tools/path-security.js') + }) + + it('does not set permissionRules when both disk and session rules are empty', async () => { + vi.doMock('../permissions/registry.js', () => ({ + loadMergedRules: vi.fn().mockResolvedValue([]), + })) + vi.doMock('../tools/path-security.js', () => ({ + getSessionAllowedRules: vi.fn().mockReturnValue([]), + clearAllowedPaths: vi.fn(), + addSessionAllowedRule: vi.fn(), + addAllowedPath: vi.fn(), + addAllowedPaths: vi.fn(), + isPathAllowed: vi.fn().mockReturnValue(false), + requestPathAccess: vi.fn(), + PathAccessDeniedError: class PathAccessDeniedError extends Error {}, + AskUserInterrupt: class AskUserInterrupt extends Error {}, + })) + + const executeToolsSpy = vi + .spyOn(await import('./execute-tools.js'), 'executeTools') + .mockImplementation(async (_msgId: string, _calls: any, ctx: any) => { + capturedBatchContext = ctx + return { toolMessages: [], criteriaChanged: false } + }) + + await runTopLevelAgentLoop(makeConfig(), mockTurnMetrics) + + expect(capturedBatchContext).toBeDefined() + expect(capturedBatchContext.permissionRules).toBeUndefined() + + executeToolsSpy.mockRestore() + vi.doUnmock('../permissions/registry.js') + vi.doUnmock('../tools/path-security.js') + }) +}) diff --git a/src/server/chat/agent-loop.ts b/src/server/chat/agent-loop.ts index ff7bce8d..fb768ff3 100644 --- a/src/server/chat/agent-loop.ts +++ b/src/server/chat/agent-loop.ts @@ -525,6 +525,14 @@ ${COMPACTION_PROMPT}`, batchContext.providerManager = config.providerManager } batchContext.agentTimeout = getRuntimeConfig().agent.toolTimeout + const { loadMergedRules } = await import('../permissions/registry.js') + const { getSessionAllowedRules } = await import('../tools/path-security.js') + const diskRules = await loadMergedRules(configDir, sessionManager.getEffectiveWorkdir(sessionId)) + const sessionRules = getSessionAllowedRules(sessionId) + const permissionRules = [...diskRules, ...sessionRules] + if (permissionRules.length > 0) { + batchContext.permissionRules = permissionRules + } const batchResult = await executeTools(assistantMsgId, result.toolCalls, batchContext, append) if (batchResult.stepDoneCalled) { emitDoneAndBreak( diff --git a/src/server/chat/dynamic-context.test.ts b/src/server/chat/dynamic-context.test.ts index 17efbf5c..71868634 100644 --- a/src/server/chat/dynamic-context.test.ts +++ b/src/server/chat/dynamic-context.test.ts @@ -217,4 +217,12 @@ describe('computeDynamicContextHash', () => { const b = computeDynamicContextHash('do foo', skills, 'tool-fp', undefined) expect(a).toBe(b) }) + + it('permissionRules no longer affect the hash (cache-safe)', () => { + const without = computeDynamicContextHash('do foo', skills, 'tool-fp') + const withEmpty = computeDynamicContextHash('do foo', skills, 'tool-fp', undefined) + const withUndefined = computeDynamicContextHash('do foo', skills, 'tool-fp') + expect(withEmpty).toBe(without) + expect(withUndefined).toBe(without) + }) }) diff --git a/src/server/chat/dynamic-context.ts b/src/server/chat/dynamic-context.ts index c412bedb..0c01378d 100644 --- a/src/server/chat/dynamic-context.ts +++ b/src/server/chat/dynamic-context.ts @@ -112,7 +112,7 @@ export function getToolFingerprint(tools: LLMToolDefinition[]): string { .join('|') } -async function loadSessionContext( +export async function loadSessionContext( sessionManager: SessionManager, sessionId: string, ): Promise<{ instructionContent: string; skills: SkillMetadata[] }> { @@ -120,7 +120,8 @@ async function loadSessionContext( const { content: instructionContent } = await getAllInstructions(session.workdir, session.projectId) const runtimeConfig = getRuntimeConfig() const configDir = getGlobalConfigDir(runtimeConfig.mode ?? 'production') - const skills = await getEnabledSkillMetadata(configDir, sessionManager.getProjectWorkdir(sessionId)) + const workdir = sessionManager.getProjectWorkdir(sessionId) + const skills = await getEnabledSkillMetadata(configDir, workdir) return { instructionContent: instructionContent ?? '', skills } } diff --git a/src/server/chat/execute-tools.test.ts b/src/server/chat/execute-tools.test.ts index 789b2ad4..b022628d 100644 --- a/src/server/chat/execute-tools.test.ts +++ b/src/server/chat/execute-tools.test.ts @@ -4,6 +4,7 @@ import type { TurnMetrics } from './stream-pure.js' import type { ToolRegistry } from '../tools/types.js' import type { TurnEvent } from '../events/types.js' import { executeTools, transformSubAgentAliases } from './execute-tools.js' +import { PathAccessDeniedError } from '../tools/path-security.js' vi.mock('../agents/registry.js', () => ({ loadAllAgentsDefault: vi.fn(), @@ -469,4 +470,118 @@ describe('executeTools', () => { expect(result.returnValueContent).toBe('my result') expect(result.returnValueResult).toBe('completed') }) + + it('shows rule_denied message for PathAccessDeniedError with rule_denied reason', async () => { + const append = vi.fn() + mockToolRegistry.execute = vi + .fn() + .mockRejectedValue( + new PathAccessDeniedError( + ['/home/tony/perso/littlehands'], + 'read_file', + 'rule_denied', + 'Permission rule DENY blocked: "/home/tony/perso/littlehands"', + ), + ) + + const toolCalls: ToolCall[] = [ + { id: 'call-1', name: 'read_file', arguments: { path: '/home/tony/perso/littlehands' } }, + ] + + const result = await executeTools('msg-1', toolCalls, makeCtx(), append) + + expect(result.toolMessages).toHaveLength(1) + expect(result.toolMessages[0]?.content).toContain('Blocked') + expect(result.toolMessages[0]?.content).toContain('blocked by a permission rule') + expect(result.toolMessages[0]?.content).not.toContain('User denied access') + }) + + it('shows outside_workdir message for PathAccessDeniedError with outside_workdir reason', async () => { + const append = vi.fn() + mockToolRegistry.execute = vi + .fn() + .mockRejectedValue(new PathAccessDeniedError(['/etc/passwd'], 'read_file', 'outside_workdir')) + + const toolCalls: ToolCall[] = [{ id: 'call-1', name: 'read_file', arguments: { path: '/etc/passwd' } }] + + const result = await executeTools('msg-1', toolCalls, makeCtx(), append) + + expect(result.toolMessages).toHaveLength(1) + expect(result.toolMessages[0]?.content).toContain('Access denied to') + expect(result.toolMessages[0]?.content).toContain('outside the project directory') + }) + + it('DENY rule on non-enforcing tool (web_fetch) blocks execution without calling the tool', async () => { + const append = vi.fn() + mockToolRegistry.execute = vi.fn().mockResolvedValue({ + success: true, + output: 'should not reach', + durationMs: 0, + truncated: false, + }) + + const rules = [{ effect: 'DENY' as const, tool: 'web_fetch' }] + const toolCalls: ToolCall[] = [{ id: 'call-1', name: 'web_fetch', arguments: { url: 'https://example.com' } }] + + const result = await executeTools('msg-1', toolCalls, makeCtx({ permissionRules: rules }), append) + + expect(mockToolRegistry.execute).not.toHaveBeenCalled() + expect(result.toolMessages).toHaveLength(1) + expect(result.toolMessages[0]?.content).toContain('blocked by a permission rule') + }) + + it('DENY rule on call_sub_agent blocks execution', async () => { + const append = vi.fn() + mockToolRegistry.execute = vi.fn().mockResolvedValue({ + success: true, + output: 'should not reach', + durationMs: 0, + truncated: false, + }) + + const rules = [{ effect: 'DENY' as const, tool: 'call_sub_agent' }] + const toolCalls: ToolCall[] = [ + { id: 'call-1', name: 'call_sub_agent', arguments: { subAgentType: 'explorer', prompt: 'test' } }, + ] + + const result = await executeTools('msg-1', toolCalls, makeCtx({ permissionRules: rules }), append) + + expect(mockToolRegistry.execute).not.toHaveBeenCalled() + expect(result.toolMessages[0]?.content).toContain('blocked by a permission rule') + }) + + it('no rule on web_fetch → tool executes normally', async () => { + const append = vi.fn() + mockToolRegistry.execute = vi.fn().mockResolvedValue({ + success: true, + output: 'fetched', + durationMs: 10, + truncated: false, + }) + + const toolCalls: ToolCall[] = [{ id: 'call-1', name: 'web_fetch', arguments: { url: 'https://example.com' } }] + + const result = await executeTools('msg-1', toolCalls, makeCtx(), append) + + expect(mockToolRegistry.execute).toHaveBeenCalledTimes(1) + expect(result.toolMessages[0]?.content).toContain('fetched') + }) + + it('ALLOW rule on non-enforcing tool does not block (no gate for ALLOW)', async () => { + const append = vi.fn() + mockToolRegistry.execute = vi.fn().mockResolvedValue({ + success: true, + output: 'ok', + durationMs: 10, + truncated: false, + }) + + const rules = [{ effect: 'ALLOW' as const, tool: 'web_fetch' }] + const toolCalls: ToolCall[] = [{ id: 'call-1', name: 'web_fetch', arguments: { url: 'https://example.com' } }] + + const result = await executeTools('msg-1', toolCalls, makeCtx({ permissionRules: rules }), append) + + expect(mockToolRegistry.execute).toHaveBeenCalledTimes(1) + expect(result.toolMessages[0]?.content).toContain('ok') + }) }) diff --git a/src/server/chat/execute-tools.ts b/src/server/chat/execute-tools.ts index 4f6a9f82..de93c386 100644 --- a/src/server/chat/execute-tools.ts +++ b/src/server/chat/execute-tools.ts @@ -12,6 +12,24 @@ import type { DangerLevel } from '../../shared/types.js' import { createToolProgressHandler } from './tool-streaming.js' import { createToolCallEvent, createToolResultEvent, createChatDoneEvent } from './stream-pure.js' import { PathAccessDeniedError, AskUserInterrupt } from '../tools/index.js' +import { evaluateRulesWithMatch } from '../permissions/rules.js' +import type { PermissionRule } from '../permissions/schema.js' + +const PATH_ENFORCING_TOOLS = new Set(['read_file', 'write_file', 'edit_file', 'run_command']) + +function evaluateToolGate(rules: PermissionRule[], toolName: string): PathAccessDeniedError | null { + if (PATH_ENFORCING_TOOLS.has(toolName)) return null + const match = evaluateRulesWithMatch(rules, toolName, '') + if (match.effect === 'DENY') { + return new PathAccessDeniedError( + [toolName], + toolName, + 'rule_denied', + `Permission rule DENY blocked tool: "${toolName}"`, + ) + } + return null +} import { loadAllAgentsDefault, findAgentById } from '../agents/registry.js' import { logger } from '../utils/logger.js' import { sanitizeUtf8 } from '../utils/utf8.js' @@ -24,6 +42,7 @@ export interface ToolBatchContext { workdir: string dangerLevel?: DangerLevel isSubAgent?: boolean + permissionRules?: PermissionRule[] turnMetrics: TurnMetrics signal?: AbortSignal | undefined onMessage?: ((msg: ServerMessage) => void) | undefined @@ -116,9 +135,20 @@ export async function executeTools( startTime: number, ): Promise => { if (error instanceof PathAccessDeniedError) { + const reasonText = + error.reason === 'rule_denied' + ? 'blocked by a permission rule' + : error.reason === 'rule_ask' + ? 'requiring confirmation per a permission rule' + : error.reason === 'git_no_verify' + ? 'git commands with --no-verify' + : error.reason === 'dangerous_command' + ? 'potentially dangerous commands' + : 'outside the project directory' + const prefix = error.reason === 'rule_denied' ? 'Blocked' : 'Access denied to' return { success: false, - error: `User denied access to ${error.paths.join(', ')}. If you need this file, explain why and ask for permission.`, + error: `${prefix} ${error.paths.join(', ')} (${reasonText}). If you need this file, explain why and ask the user for permission.`, durationMs: Date.now() - startTime, truncated: false, } @@ -216,10 +246,19 @@ export async function executeTools( if (ctx.providerManager) { toolContext.providerManager = ctx.providerManager } + if (ctx.permissionRules && ctx.permissionRules.length > 0) { + toolContext.permissionRules = ctx.permissionRules + } const startTime = Date.now() let toolResult: ToolResult try { + if (ctx.permissionRules && ctx.permissionRules.length > 0) { + const gateResult = evaluateToolGate(ctx.permissionRules, toolCall.name) + if (gateResult) { + throw gateResult + } + } toolResult = await ctx.toolRegistry.execute(toolCall.name, toolCall.arguments, toolContext) } catch (error) { toolResult = await handleToolExecutionError(error, ctx.sessionId, startTime) diff --git a/src/server/chat/orchestrator.test.ts b/src/server/chat/orchestrator.test.ts index 96de93b5..0892679c 100644 --- a/src/server/chat/orchestrator.test.ts +++ b/src/server/chat/orchestrator.test.ts @@ -891,7 +891,8 @@ describe('chat orchestrator', () => { data: { result: { success: false, - error: 'User denied access to /etc/passwd. If you need this file, explain why and ask for permission.', + error: + 'Access denied to /etc/passwd (outside the project directory). If you need this file, explain why and ask the user for permission.', }, }, }) diff --git a/src/server/chat/orchestrator.ts b/src/server/chat/orchestrator.ts index 45f7365f..d2095766 100644 --- a/src/server/chat/orchestrator.ts +++ b/src/server/chat/orchestrator.ts @@ -30,13 +30,14 @@ import { } from './stream-pure.js' import { createAssemblyResult } from './request-context.js' import type { RequestContextMessage } from './request-context.js' -import { buildCachedPrompt, computeDynamicContextHash, getToolFingerprint } from './dynamic-context.js' +import { + buildCachedPrompt, + computeDynamicContextHash, + getToolFingerprint, + loadSessionContext, +} from './dynamic-context.js' import { runTopLevelAgentLoop } from './agent-loop.js' import { loadAllAgentsDefault, findAgentById, resolveDefaultAgentId } from '../agents/registry.js' -import { getAllInstructions } from '../context/instructions.js' -import { getEnabledSkillMetadata } from '../skills/registry.js' -import { getRuntimeConfig } from '../runtime-config.js' -import { getGlobalConfigDir } from '../../cli/paths.js' import { logger } from '../utils/logger.js' import type { RetryPatternConfig } from './auto-patterns.js' import { getConversationMessages, processEventsForConversation } from './conversation-history.js' @@ -189,11 +190,20 @@ export async function runChatTurn(options: OrchestratorOptions): Promise { ? 'sensitive files that may contain secrets' : error.reason === 'both' ? 'files outside the project and sensitive files' - : 'files outside the project directory' + : error.reason === 'rule_denied' + ? 'paths/commands blocked by a permission rule' + : error.reason === 'rule_ask' + ? 'paths/commands requiring confirmation per a permission rule' + : error.reason === 'git_no_verify' + ? 'git commands with --no-verify' + : error.reason === 'dangerous_command' + ? 'potentially dangerous commands' + : 'files outside the project directory' + const errorPrefix = error.reason === 'rule_denied' ? 'Blocked' : 'User denied access to' eventStore.append(sessionId, { type: 'chat.error', data: { - error: `User denied access to ${reasonText}.`, + error: `${errorPrefix} ${reasonText}.`, recoverable: false, }, }) @@ -352,12 +362,7 @@ export async function runAgentTurn( injectAgentReminder(options.sessionId, agentDef) } - const session = options.sessionManager.requireSession(options.sessionId) - - const { content: instructionContent } = await getAllInstructions(session.workdir, session.projectId) - const runtimeConfig = getRuntimeConfig() - const configDir = getGlobalConfigDir(runtimeConfig.mode ?? 'production') - const skills = await getEnabledSkillMetadata(configDir, options.sessionManager.getProjectWorkdir(options.sessionId)) + const { instructionContent, skills } = await loadSessionContext(options.sessionManager, options.sessionId) return runTopLevelAgentLoop( { @@ -376,7 +381,7 @@ export async function runAgentTurn( if (cached) { const toolFingerprint = getToolFingerprint(cached.tools) const currentHash = computeDynamicContextHash( - instructionContent ?? '', + instructionContent, skills, toolFingerprint, agentLlmClient.getModel(), diff --git a/src/server/chat/prompts.permissions.test.ts b/src/server/chat/prompts.permissions.test.ts new file mode 100644 index 00000000..98de8441 --- /dev/null +++ b/src/server/chat/prompts.permissions.test.ts @@ -0,0 +1,46 @@ +import { describe, it, expect } from 'vitest' +import { buildTopLevelSystemPrompt } from './prompts.js' +import type { SkillMetadata } from '../skills/types.js' +import type { AgentDefinition } from '../agents/types.js' + +describe('buildTopLevelSystemPrompt — cache-safety contract', () => { + const skills: SkillMetadata[] = [] + const subAgentDefs: AgentDefinition[] = [] + + it('does not include a PERMISSIONS section in the prompt', () => { + const prompt = buildTopLevelSystemPrompt('/tmp', undefined, skills, subAgentDefs) + expect(prompt).not.toContain('## PERMISSIONS') + expect(prompt).not.toMatch(/PERMISSIONS/) + }) + + it('is byte-identical whether modelName is omitted or undefined', () => { + const a = buildTopLevelSystemPrompt('/tmp', undefined, skills, subAgentDefs) + const b = buildTopLevelSystemPrompt('/tmp', undefined, skills, subAgentDefs, undefined) + expect(b).toBe(a) + }) + + it('preserves base prompt contract (working directory, system-reminder override)', () => { + const prompt = buildTopLevelSystemPrompt('/old/workdir', undefined, skills, subAgentDefs) + expect(prompt).toContain('Working directory: /old/workdir') + expect(prompt).toMatch(/working directory[^.\n]*\b(may|can)\b[^.\n]*change/i) + expect(prompt).toMatch(/[^]*?trust[^]*?(workspace|that value|over this)/i) + expect(prompt).toContain('authoritative') + expect(prompt).toContain('operational constraints') + }) + + it('sub-agents section still present (no permissions section appended)', () => { + const subAgent: AgentDefinition = { + metadata: { + id: 'verifier', + name: 'Verifier', + description: 'Verifies', + subagent: true, + allowedTools: ['read_file'], + }, + prompt: 'Verify.', + } + const prompt = buildTopLevelSystemPrompt('/tmp', undefined, skills, [subAgent]) + expect(prompt).toContain('AVAILABLE SUB-AGENTS') + expect(prompt).not.toContain('## PERMISSIONS') + }) +}) diff --git a/src/server/chat/prompts.ts b/src/server/chat/prompts.ts index b7e51a57..5c318db3 100644 --- a/src/server/chat/prompts.ts +++ b/src/server/chat/prompts.ts @@ -186,6 +186,10 @@ To call a sub-agent, use the call_sub_agent tool with: * System prompt for top-level agents (planner, builder, custom). * Identical for all top-level agents to preserve KV cache. * Agent-specific behavior comes from the runtime reminder. + * + * Permission rules are NOT included in the prompt: they are enforced + * deterministically by the server (path-security.ts) and including them + * here would invalidate the KV cache on every rule edit. */ export function buildTopLevelSystemPrompt( workdir: string, diff --git a/src/server/dev-server/inspect-proxy.test.ts b/src/server/dev-server/inspect-proxy.test.ts index c18c4bcd..2edffced 100644 --- a/src/server/dev-server/inspect-proxy.test.ts +++ b/src/server/dev-server/inspect-proxy.test.ts @@ -51,6 +51,9 @@ function httpGetRaw( }) }) req.on('error', reject) + req.setTimeout(10_000, () => { + req.destroy(new Error('httpGetRaw timeout')) + }) req.end() }) } @@ -527,6 +530,6 @@ describe('InspectProxy', () => { } finally { cleanup() } - }) + }, 60_000) }) }) diff --git a/src/server/events/types.ts b/src/server/events/types.ts index 9124bc8f..62bbef9e 100644 --- a/src/server/events/types.ts +++ b/src/server/events/types.ts @@ -405,7 +405,14 @@ export type TurnEvent = tool: string paths: string[] workdir: string - reason: 'outside_workdir' | 'sensitive_file' | 'both' | 'dangerous_command' | 'git_no_verify' + reason: + | 'outside_workdir' + | 'sensitive_file' + | 'both' + | 'dangerous_command' + | 'git_no_verify' + | 'rule_denied' + | 'rule_ask' } } | { @@ -522,7 +529,8 @@ export interface PendingPathConfirmation { tool: string paths: string[] workdir: string - reason: 'outside_workdir' | 'sensitive_file' | 'both' | 'dangerous_command' | 'git_no_verify' + reason: + 'outside_workdir' | 'sensitive_file' | 'both' | 'dangerous_command' | 'git_no_verify' | 'rule_denied' | 'rule_ask' } export interface CompactionRecord { diff --git a/src/server/index.ts b/src/server/index.ts index 3413d9c6..362177a2 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -42,6 +42,7 @@ import { loadAllAgentsDefault, getTopLevelAgents } from './agents/registry.js' import { createWorkflowRoutes } from './routes/workflows.js' import { createDevServerRoutes } from './routes/dev-server.js' import { createWorkspaceConfigRoutes } from './routes/workspace-config.js' +import { createPermissionsRoutes } from './routes/permissions.js' import { createTerminalRoutes } from './routes/terminals.js' import { WorkspaceInUseError } from './utils/errors.js' import { createDirectoryRoutes } from './routes/directories.js' @@ -748,7 +749,14 @@ export async function createServerHandle(config: Config): Promise tool: string paths: string[] workdir: string - reason: 'outside_workdir' | 'sensitive_file' | 'both' | 'dangerous_command' | 'git_no_verify' + reason: + | 'outside_workdir' + | 'sensitive_file' + | 'both' + | 'dangerous_command' + | 'git_no_verify' + | 'rule_denied' + | 'rule_ask' }> > = {} for (const s of sessions) { @@ -944,13 +952,15 @@ export async function createServerHandle(config: Config): Promise // Cancel any active execution before deleting — mirrors /stop endpoint const { stopSessionExecution } = await import('./session/chat-handler.js') - const { cancelQuestionsForSession, cancelPathConfirmationsForSession } = await import('./tools/index.js') + const { cancelQuestionsForSession, cancelPathConfirmationsForSession, clearAllowedPaths } = + await import('./tools/index.js') sessionManager.clearMessageQueue(sessionId) stopSessionExecution(sessionId, sessionManager) abortSession(sessionId) cancelQuestionsForSession(sessionId, 'Session deleted') cancelPathConfirmationsForSession(sessionId, 'Session deleted') + clearAllowedPaths(sessionId) sessionManager.deleteSession(sessionId) wssExports.broadcastAll({ @@ -1460,7 +1470,8 @@ export async function createServerHandle(config: Config): Promise } const { stopSessionExecution } = await import('./session/chat-handler.js') - const { cancelQuestionsForSession, cancelPathConfirmationsForSession } = await import('./tools/index.js') + const { cancelQuestionsForSession, cancelPathConfirmationsForSession, clearAllowedPaths } = + await import('./tools/index.js') // Drain queued messages BEFORE stopping execution, so the QueueProcessor // doesn't pick them up when running_changed fires from setRunning(false) @@ -1473,6 +1484,7 @@ export async function createServerHandle(config: Config): Promise cancelQuestionsForSession(sessionId, 'Session stopped by user') cancelPathConfirmationsForSession(sessionId, 'Session stopped by user') + clearAllowedPaths(sessionId) const eventStore = (await import('./events/index.js')).getEventStore() eventStore.append(sessionId, { type: 'running.changed', data: { isRunning: false } }) @@ -2983,6 +2995,7 @@ export async function createServerHandle(config: Config): Promise app.use('/api/workflows', createWorkflowRoutes(configDir, config, projectDir)) app.use('/api/dev-server', createDevServerRoutes()) app.use('/api/workspace', createWorkspaceConfigRoutes(sessionManager)) + app.use('/api/permissions', createPermissionsRoutes(configDir)) app.use('/api/terminals', createTerminalRoutes()) app.use( '/api/auto-update', diff --git a/src/server/llm/mock-rules.ts b/src/server/llm/mock-rules.ts index ddb08dcc..84bd8798 100644 --- a/src/server/llm/mock-rules.ts +++ b/src/server/llm/mock-rules.ts @@ -398,6 +398,16 @@ export const RULES: MockRule[] = [ tools: [{ name: 'run_command', arguments: { command: 'echo "Hello World"' } }], response: 'Executed echo command.', }, + { + match: /run.*terragrunt.*destroy/i, + tools: [{ name: 'run_command', arguments: { command: 'terragrunt destroy /tmp/test' } }], + response: 'Ran terragrunt destroy.', + }, + { + match: /run.*rm.*-rf.*root/i, + tools: [{ name: 'run_command', arguments: { command: 'rm -rf /' } }], + response: 'Attempted to delete root.', + }, { match: /run.*cat.*package\.json/i, tools: [{ name: 'run_command', arguments: { command: 'cat package.json' } }], diff --git a/src/server/permissions/registry.test.ts b/src/server/permissions/registry.test.ts new file mode 100644 index 00000000..91669ec6 --- /dev/null +++ b/src/server/permissions/registry.test.ts @@ -0,0 +1,192 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' +import { mkdir, rm, writeFile } from 'node:fs/promises' +import { join } from 'node:path' +import { tmpdir } from 'node:os' +import { + loadPermissionsConfig, + savePermissionsConfig, + loadMergedRules, + getGlobalPermissionsPath, + getProjectPermissionsPath, +} from './registry.js' + +const loggerMock = vi.hoisted(() => ({ + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), +})) + +vi.mock('../utils/logger.js', () => ({ + logger: loggerMock, +})) + +const TEST_DIR = join(tmpdir(), 'openfox-permissions-registry-test') +const GLOBAL_DIR = join(TEST_DIR, 'global') +const PROJECT_DIR = join(TEST_DIR, 'project') + +beforeEach(async () => { + await rm(TEST_DIR, { recursive: true, force: true }) + await mkdir(GLOBAL_DIR, { recursive: true }) + await mkdir(PROJECT_DIR, { recursive: true }) + await mkdir(join(PROJECT_DIR, '.openfox'), { recursive: true }) + loggerMock.warn.mockClear() +}) + +afterEach(async () => { + await rm(TEST_DIR, { recursive: true, force: true }) +}) + +describe('getGlobalPermissionsPath / getProjectPermissionsPath', () => { + it('returns configDir/permissions.json for global', () => { + expect(getGlobalPermissionsPath(GLOBAL_DIR)).toBe(join(GLOBAL_DIR, 'permissions.json')) + }) + + it('returns workdir/.openfox/permissions.json for project', () => { + expect(getProjectPermissionsPath(PROJECT_DIR)).toBe(join(PROJECT_DIR, '.openfox', 'permissions.json')) + }) +}) + +describe('loadPermissionsConfig', () => { + it('returns empty config when file does not exist', async () => { + const config = await loadPermissionsConfig('global', GLOBAL_DIR, PROJECT_DIR) + expect(config).toEqual({ version: 1, rules: [] }) + }) + + it('loads a valid config file', async () => { + const config = { version: 1, rules: [{ effect: 'DENY', tool: 'run_command', pattern: 'rm -rf *' }] } + await writeFile(join(GLOBAL_DIR, 'permissions.json'), JSON.stringify(config)) + const loaded = await loadPermissionsConfig('global', GLOBAL_DIR, PROJECT_DIR) + expect(loaded).toEqual(config) + }) + + it('loads project config from .openfox/permissions.json', async () => { + const config = { + version: 1, + rules: [{ effect: 'ALLOW', tool: 'read_file', pattern: '/ubiquity/**' }], + } + await writeFile(join(PROJECT_DIR, '.openfox', 'permissions.json'), JSON.stringify(config)) + const loaded = await loadPermissionsConfig('project', GLOBAL_DIR, PROJECT_DIR) + expect(loaded).toEqual(config) + }) + + it('returns empty config on invalid JSON (graceful)', async () => { + await writeFile(join(GLOBAL_DIR, 'permissions.json'), '{ not valid json') + const loaded = await loadPermissionsConfig('global', GLOBAL_DIR, PROJECT_DIR) + expect(loaded).toEqual({ version: 1, rules: [] }) + }) + + it('logs warning on invalid JSON parse error', async () => { + await writeFile(join(GLOBAL_DIR, 'permissions.json'), '{ not valid json') + await loadPermissionsConfig('global', GLOBAL_DIR, PROJECT_DIR) + expect(loggerMock.warn).toHaveBeenCalledWith( + 'permissions.json parse error, ignoring', + expect.objectContaining({ path: join(GLOBAL_DIR, 'permissions.json') }), + ) + }) + + it('does NOT log warning when file does not exist (ENOENT)', async () => { + await loadPermissionsConfig('global', GLOBAL_DIR, PROJECT_DIR) + expect(loggerMock.warn).not.toHaveBeenCalled() + }) + + it('returns empty config on Zod validation failure', async () => { + await writeFile(join(GLOBAL_DIR, 'permissions.json'), JSON.stringify({ version: 1, rules: [{ effect: 'BAD' }] })) + const loaded = await loadPermissionsConfig('global', GLOBAL_DIR, PROJECT_DIR) + expect(loaded).toEqual({ version: 1, rules: [] }) + }) +}) + +describe('savePermissionsConfig', () => { + it('saves and reloads identical config (round-trip)', async () => { + const config = { + version: 1 as const, + rules: [ + { effect: 'DENY' as const, tool: 'run_command', pattern: 'rm -rf *' }, + { effect: 'ALLOW' as const, tool: 'read_file', pattern: '/ubiquity/**' }, + ], + } + await savePermissionsConfig('global', GLOBAL_DIR, PROJECT_DIR, config) + const loaded = await loadPermissionsConfig('global', GLOBAL_DIR, PROJECT_DIR) + expect(loaded).toEqual(config) + }) + + it('deletes the file when rules array is empty', async () => { + const config = { version: 1 as const, rules: [] } + await savePermissionsConfig('global', GLOBAL_DIR, PROJECT_DIR, config) + const { stat } = await import('node:fs/promises') + await expect(stat(join(GLOBAL_DIR, 'permissions.json'))).rejects.toThrow() + }) + + it('creates .openfox dir if missing for project scope', async () => { + const newProject = join(TEST_DIR, 'new-project') + await mkdir(newProject, { recursive: true }) + const config = { + version: 1 as const, + rules: [{ effect: 'DENY' as const, tool: 'write_file', pattern: '**/.env*' }], + } + await savePermissionsConfig('project', GLOBAL_DIR, newProject, config) + const loaded = await loadPermissionsConfig('project', GLOBAL_DIR, newProject) + expect(loaded).toEqual(config) + }) +}) + +describe('loadMergedRules', () => { + it('returns empty array when no files exist', async () => { + const rules = await loadMergedRules(GLOBAL_DIR, PROJECT_DIR) + expect(rules).toEqual([]) + }) + + it('returns only global rules when no project file', async () => { + const globalConfig = { + version: 1 as const, + rules: [{ effect: 'DENY' as const, tool: 'run_command', pattern: 'rm -rf *' }], + } + await writeFile(join(GLOBAL_DIR, 'permissions.json'), JSON.stringify(globalConfig)) + const rules = await loadMergedRules(GLOBAL_DIR, PROJECT_DIR) + expect(rules).toHaveLength(1) + expect(rules[0]!.effect).toBe('DENY') + }) + + it('merges global + project rules', async () => { + await writeFile( + join(GLOBAL_DIR, 'permissions.json'), + JSON.stringify({ + version: 1, + rules: [{ effect: 'DENY', tool: 'run_command', pattern: 'rm -rf *' }], + }), + ) + await writeFile( + join(PROJECT_DIR, '.openfox', 'permissions.json'), + JSON.stringify({ + version: 1, + rules: [{ effect: 'ALLOW', tool: 'read_file', pattern: '/ubiquity/**' }], + }), + ) + const rules = await loadMergedRules(GLOBAL_DIR, PROJECT_DIR) + expect(rules).toHaveLength(2) + expect(rules.map((r) => r.effect)).toContain('DENY') + expect(rules.map((r) => r.effect)).toContain('ALLOW') + }) + + it('re-reads file on each load (no stale cache)', async () => { + await writeFile( + join(GLOBAL_DIR, 'permissions.json'), + JSON.stringify({ version: 1, rules: [{ effect: 'DENY', tool: 'read_file' }] }), + ) + const rules1 = await loadMergedRules(GLOBAL_DIR, PROJECT_DIR) + expect(rules1).toHaveLength(1) + await writeFile( + join(GLOBAL_DIR, 'permissions.json'), + JSON.stringify({ + version: 1, + rules: [ + { effect: 'DENY', tool: 'read_file' }, + { effect: 'ALLOW', tool: 'read_file' }, + ], + }), + ) + const rules2 = await loadMergedRules(GLOBAL_DIR, PROJECT_DIR) + expect(rules2).toHaveLength(2) + }) +}) diff --git a/src/server/permissions/registry.ts b/src/server/permissions/registry.ts new file mode 100644 index 00000000..c068e982 --- /dev/null +++ b/src/server/permissions/registry.ts @@ -0,0 +1,64 @@ +import { readFile, writeFile, mkdir, rm } from 'node:fs/promises' +import { resolve, join } from 'node:path' +import { permissionConfigSchema, EMPTY_CONFIG, type PermissionConfig, type PermissionRule } from './schema.js' +import { logger } from '../utils/logger.js' + +export type PermissionsScope = 'global' | 'project' + +export function getGlobalPermissionsPath(configDir: string): string { + return join(resolve(configDir), 'permissions.json') +} + +export function getProjectPermissionsPath(workdir: string): string { + return join(resolve(workdir), '.openfox', 'permissions.json') +} + +function getPath(scope: PermissionsScope, configDir: string, workdir: string): string { + return scope === 'global' ? getGlobalPermissionsPath(configDir) : getProjectPermissionsPath(workdir) +} + +export async function loadPermissionsConfig( + scope: PermissionsScope, + configDir: string, + workdir: string, +): Promise { + const path = getPath(scope, configDir, workdir) + try { + const raw = await readFile(path, 'utf-8') + const parsed = JSON.parse(raw) + return permissionConfigSchema.parse(parsed) + } catch (err) { + if (err instanceof Error && 'issues' in err) { + logger.warn('permissions.json validation failed, ignoring', { path, error: String(err) }) + } else if (err instanceof Error && (err as NodeJS.ErrnoException).code !== 'ENOENT') { + logger.warn('permissions.json parse error, ignoring', { path, error: err.message }) + } + return EMPTY_CONFIG + } +} + +export async function savePermissionsConfig( + scope: PermissionsScope, + configDir: string, + workdir: string, + config: PermissionConfig, +): Promise { + const path = getPath(scope, configDir, workdir) + if (config.rules.length === 0) { + await rm(path, { force: true }) + return + } + if (scope === 'project') { + const dir = join(resolve(workdir), '.openfox') + await mkdir(dir, { recursive: true }) + } else { + await mkdir(resolve(configDir), { recursive: true }) + } + await writeFile(path, JSON.stringify(config, null, 2) + '\n', 'utf-8') +} + +export async function loadMergedRules(configDir: string, workdir: string): Promise { + const globalConfig = await loadPermissionsConfig('global', configDir, workdir) + const projectConfig = await loadPermissionsConfig('project', configDir, workdir) + return [...globalConfig.rules, ...projectConfig.rules] +} diff --git a/src/server/permissions/rules.test.ts b/src/server/permissions/rules.test.ts new file mode 100644 index 00000000..030bf957 --- /dev/null +++ b/src/server/permissions/rules.test.ts @@ -0,0 +1,190 @@ +import { describe, it, expect } from 'vitest' +import { evaluateRules, evaluateRulesWithMatch, matchPathPattern, matchCommandPattern } from './rules.js' +import type { PermissionRule } from './schema.js' + +describe('matchPathPattern', () => { + it('matches ** across directory boundaries', () => { + expect(matchPathPattern('/ubiquity/**', '/ubiquity/a/b.yaml')).toBe(true) + expect(matchPathPattern('/ubiquity/**', '/ubiquity/a/b/c.yaml')).toBe(true) + expect(matchPathPattern('/ubiquity/**', '/ubiquity')).toBe(true) + }) + + it('does not match outside the pattern', () => { + expect(matchPathPattern('/ubiquity/**', '/other/a.yaml')).toBe(false) + expect(matchPathPattern('/ubiquity/**', '/ubiquity-other/x')).toBe(false) + }) + + it('matches * within one segment', () => { + expect(matchPathPattern('/ubiquity/*.yaml', '/ubiquity/a.yaml')).toBe(true) + expect(matchPathPattern('/ubiquity/*.yaml', '/ubiquity/sub/a.yaml')).toBe(false) + }) + + it('empty/undefined pattern matches everything', () => { + expect(matchPathPattern(undefined, '/any/path')).toBe(true) + expect(matchPathPattern('', '/any/path')).toBe(true) + }) + + it('trailing slash normalization', () => { + expect(matchPathPattern('/ubiquity/**', '/ubiquity/')).toBe(true) + }) +}) + +describe('matchCommandPattern', () => { + it('matches rm -rf with any argument', () => { + expect(matchCommandPattern('rm -rf *', 'rm -rf /tmp/x')).toBe(true) + expect(matchCommandPattern('rm -rf *', 'rm -rf /home/user/foo')).toBe(true) + }) + + it('does not match rm -rf alone (needs arg)', () => { + expect(matchCommandPattern('rm -rf *', 'rm -rf')).toBe(false) + }) + + it('does not match different command', () => { + expect(matchCommandPattern('rm -rf *', 'ls -la')).toBe(false) + }) + + it('exact command match', () => { + expect(matchCommandPattern('sudo apt update', 'sudo apt update')).toBe(true) + expect(matchCommandPattern('sudo apt update', 'sudo apt upgrade')).toBe(false) + }) + + it('empty/undefined pattern matches everything', () => { + expect(matchCommandPattern(undefined, 'any command here')).toBe(true) + expect(matchCommandPattern('', 'any command here')).toBe(true) + }) + + it('matches as glob where spaces are literal', () => { + expect(matchCommandPattern('git push *', 'git push origin main')).toBe(true) + expect(matchCommandPattern('git push *', 'git commit -m "x"')).toBe(false) + }) +}) + +describe('evaluateRules', () => { + const rule = (effect: PermissionRule['effect'], tool: string, pattern?: string): PermissionRule => ({ + effect, + tool, + ...(pattern !== undefined ? { pattern } : {}), + }) + + it('returns null when no rules match', () => { + expect(evaluateRules([], 'read_file', '/path')).toBeNull() + expect(evaluateRules([rule('DENY', 'write_file', '/x')], 'read_file', '/path')).toBeNull() + }) + + it('returns null when tool name does not match', () => { + expect(evaluateRules([rule('DENY', 'run_command', 'rm *')], 'read_file', '/path')).toBeNull() + }) + + it('returns the effect when a single rule matches', () => { + expect(evaluateRules([rule('DENY', 'read_file', '/secret/**')], 'read_file', '/secret/key.pem')).toBe('DENY') + expect(evaluateRules([rule('ALLOW', 'read_file', '/ubiquity/**')], 'read_file', '/ubiquity/a.yaml')).toBe('ALLOW') + expect(evaluateRules([rule('ASK', 'write_file', '**/.env*')], 'write_file', '/proj/.env')).toBe('ASK') + }) + + it('rule without pattern matches all calls to that tool', () => { + expect(evaluateRules([rule('ALLOW', 'read_file')], 'read_file', '/any/path')).toBe('ALLOW') + expect(evaluateRules([rule('DENY', 'run_command')], 'run_command', 'any command')).toBe('DENY') + }) + + it('DENY wins over ALLOW (deny > allow > ask)', () => { + const rules = [rule('ALLOW', 'read_file', '/ubiquity/**'), rule('DENY', 'read_file', '/ubiquity/secrets/**')] + expect(evaluateRules(rules, 'read_file', '/ubiquity/secrets/key.pem')).toBe('DENY') + }) + + it('DENY wins over ASK', () => { + const rules = [rule('ASK', 'read_file', '/x/**'), rule('DENY', 'read_file', '/x/**')] + expect(evaluateRules(rules, 'read_file', '/x/a')).toBe('DENY') + }) + + it('ALLOW wins over ASK', () => { + const rules = [rule('ASK', 'read_file', '/x/**'), rule('ALLOW', 'read_file', '/x/**')] + expect(evaluateRules(rules, 'read_file', '/x/a')).toBe('ALLOW') + }) + + it('global ALLOW + project DENY → DENY (deny always wins regardless of source)', () => { + const globalRules = [rule('ALLOW', 'read_file', '/ubiquity/**')] + const projectRules = [rule('DENY', 'read_file', '/ubiquity/secrets/**')] + const merged = [...globalRules, ...projectRules] + expect(evaluateRules(merged, 'read_file', '/ubiquity/secrets/key.pem')).toBe('DENY') + }) + + it('global DENY + project ALLOW → DENY', () => { + const globalRules = [rule('DENY', 'run_command', 'rm -rf *')] + const projectRules = [rule('ALLOW', 'run_command', 'rm -rf /tmp/*')] + const merged = [...globalRules, ...projectRules] + expect(evaluateRules(merged, 'run_command', 'rm -rf /tmp/x')).toBe('DENY') + }) + + it('project ALLOW + global ASK → ALLOW', () => { + const globalRules = [rule('ASK', 'read_file', '/x')] + const projectRules = [rule('ALLOW', 'read_file', '/x')] + const merged = [...globalRules, ...projectRules] + expect(evaluateRules(merged, 'read_file', '/x')).toBe('ALLOW') + }) + + it('first defined wins when multiple rules of same effect match', () => { + const rules = [rule('ALLOW', 'read_file', '/a'), rule('ALLOW', 'read_file', '/a/b')] + expect(evaluateRules(rules, 'read_file', '/a/b')).toBe('ALLOW') + }) + + it('command patterns: DENY rm -rf * blocks the command', () => { + const rules = [rule('DENY', 'run_command', 'rm -rf *')] + expect(evaluateRules(rules, 'run_command', 'rm -rf /home/user')).toBe('DENY') + }) + + it('command patterns: ALLOW does not block', () => { + const rules = [rule('ALLOW', 'run_command', 'git push *')] + expect(evaluateRules(rules, 'run_command', 'git push origin main')).toBe('ALLOW') + }) +}) + +describe('evaluateRulesWithMatch', () => { + const rule = (effect: PermissionRule['effect'], tool: string, pattern?: string): PermissionRule => ({ + effect, + tool, + ...(pattern !== undefined ? { pattern } : {}), + }) + + it('returns null effect and null rule when no rules match', () => { + const result = evaluateRulesWithMatch([], 'read_file', '/path') + expect(result.effect).toBeNull() + expect(result.rule).toBeNull() + }) + + it('returns the matched rule for DENY', () => { + const r = rule('DENY', 'read_file', '/secret/**') + const result = evaluateRulesWithMatch([r], 'read_file', '/secret/key.pem') + expect(result.effect).toBe('DENY') + expect(result.rule).toBe(r) + }) + + it('returns the matched rule for ASK', () => { + const r = rule('ASK', 'run_command', 'terragrunt destroy *') + const result = evaluateRulesWithMatch([r], 'run_command', 'terragrunt destroy -auto-approve') + expect(result.effect).toBe('ASK') + expect(result.rule).toBe(r) + }) + + it('returns the matched rule for ALLOW', () => { + const r = rule('ALLOW', 'read_file', '/x/**') + const result = evaluateRulesWithMatch([r], 'read_file', '/x/a') + expect(result.effect).toBe('ALLOW') + expect(result.rule).toBe(r) + }) + + it('DENY rule wins and is returned over ALLOW', () => { + const allowRule = rule('ALLOW', 'read_file', '/x/**') + const denyRule = rule('DENY', 'read_file', '/x/secrets/**') + const result = evaluateRulesWithMatch([allowRule, denyRule], 'read_file', '/x/secrets/key') + expect(result.effect).toBe('DENY') + expect(result.rule).toBe(denyRule) + }) + + it('returns the highest-precedence rule even if a lower one appears later', () => { + const askRule = rule('ASK', 'read_file', '/x/**') + const denyRule = rule('DENY', 'read_file', '/x/**') + const result = evaluateRulesWithMatch([askRule, denyRule], 'read_file', '/x/a') + expect(result.effect).toBe('DENY') + expect(result.rule).toBe(denyRule) + }) +}) diff --git a/src/server/permissions/rules.ts b/src/server/permissions/rules.ts new file mode 100644 index 00000000..cd6dde88 --- /dev/null +++ b/src/server/permissions/rules.ts @@ -0,0 +1,92 @@ +import { minimatch } from 'minimatch' +import type { PermissionEffect, PermissionRule } from './schema.js' + +export type { PermissionRule } from './schema.js' + +const PRECEDENCE: Record = { DENY: 3, ALLOW: 2, ASK: 1 } + +export function matchPathPattern(pattern: string | undefined, path: string): boolean { + if (!pattern) return true + const normalizedPath = path.replace(/\/+$/, '') || '/' + if (minimatch(normalizedPath, pattern, { dot: true })) return true + if (pattern.endsWith('/**')) { + const base = pattern.slice(0, -3) + return normalizedPath === base + } + if (pattern.endsWith('/**/')) { + const base = pattern.slice(0, -4) + return normalizedPath === base + } + return false +} + +function globToRegex(pattern: string): RegExp { + let regex = '^' + let i = 0 + while (i < pattern.length) { + const c = pattern[i]! + if (c === '*') { + if (pattern[i + 1] === '*') { + regex += '.*' + i += 2 + } else { + regex += '.*' + i += 1 + } + } else if (c === '?') { + regex += '.' + i += 1 + } else if ('.+^${}()|[]\\'.includes(c)) { + regex += '\\' + c + i += 1 + } else { + regex += c + i += 1 + } + } + return new RegExp(regex + '$') +} + +export function matchCommandPattern(pattern: string | undefined, command: string): boolean { + if (!pattern) return true + return globToRegex(pattern).test(command) +} + +function isPathTool(tool: string): boolean { + return tool === 'read_file' || tool === 'write_file' || tool === 'edit_file' +} + +function matchesRule(rule: PermissionRule, tool: string, target: string): boolean { + if (rule.tool !== tool) return false + const pattern = rule.pattern + if (isPathTool(tool)) return matchPathPattern(pattern, target) + if (tool === 'run_command') return matchCommandPattern(pattern, target) + return matchPathPattern(pattern, target) +} + +export function evaluateRules(rules: PermissionRule[], tool: string, target: string): PermissionEffect | null { + return evaluateRulesWithMatch(rules, tool, target).effect +} + +export interface RuleMatchResult { + effect: PermissionEffect | null + rule: PermissionRule | null +} + +export function evaluateRulesWithMatch(rules: PermissionRule[], tool: string, target: string): RuleMatchResult { + let best: PermissionEffect | null = null + let bestPrecedence = 0 + let bestRule: PermissionRule | null = null + + for (const rule of rules) { + if (!matchesRule(rule, tool, target)) continue + const precedence = PRECEDENCE[rule.effect] + if (precedence > bestPrecedence) { + best = rule.effect + bestPrecedence = precedence + bestRule = rule + } + } + + return { effect: best, rule: bestRule } +} diff --git a/src/server/permissions/schema.test.ts b/src/server/permissions/schema.test.ts new file mode 100644 index 00000000..99fa48eb --- /dev/null +++ b/src/server/permissions/schema.test.ts @@ -0,0 +1,113 @@ +import { describe, it, expect } from 'vitest' +import { permissionRuleSchema, permissionConfigSchema, type PermissionEffect } from './schema.js' + +describe('permissionRuleSchema', () => { + it('accepts a full rule with all fields', () => { + const rule = { + effect: 'DENY', + tool: 'run_command', + pattern: 'rm -rf *', + description: 'Never delete recursively', + } + expect(permissionRuleSchema.parse(rule)).toEqual(rule) + }) + + it('accepts a rule without pattern (matches all calls to that tool)', () => { + const rule = { effect: 'ALLOW', tool: 'read_file' } + const parsed = permissionRuleSchema.parse(rule) + expect(parsed.effect).toBe('ALLOW') + expect(parsed.tool).toBe('read_file') + expect(parsed.pattern).toBeUndefined() + }) + + it('accepts a rule without description', () => { + const rule = { effect: 'ASK', tool: 'write_file', pattern: '**/.env*' } + const parsed = permissionRuleSchema.parse(rule) + expect(parsed.description).toBeUndefined() + }) + + it('rejects unknown effect', () => { + expect(() => permissionRuleSchema.parse({ effect: 'ALWAYS', tool: 'read_file' })).toThrow() + expect(() => permissionRuleSchema.parse({ effect: 'allow', tool: 'read_file' })).toThrow() + expect(() => permissionRuleSchema.parse({ effect: 'MAYBE', tool: 'read_file' })).toThrow() + }) + + it('accepts unknown tool names (forward-compat with MCP/future tools) without pattern', () => { + const rule = { effect: 'DENY', tool: 'mcp_custom_tool' } + expect(permissionRuleSchema.parse(rule)).toEqual(rule) + }) + + it('rejects non-DENY effect on non-pattern tools (web_fetch)', () => { + expect(() => permissionRuleSchema.parse({ effect: 'ALLOW', tool: 'web_fetch' })).toThrow() + expect(() => permissionRuleSchema.parse({ effect: 'ASK', tool: 'web_fetch' })).toThrow() + }) + + it('rejects pattern on non-pattern tools (web_fetch)', () => { + expect(() => permissionRuleSchema.parse({ effect: 'DENY', tool: 'web_fetch', pattern: '*' })).toThrow() + }) + + it('accepts DENY without pattern on non-pattern tools', () => { + const rule = { effect: 'DENY', tool: 'web_fetch' } + expect(permissionRuleSchema.parse(rule)).toEqual(rule) + }) + + it('rejects missing effect', () => { + expect(() => permissionRuleSchema.parse({ tool: 'read_file' })).toThrow() + }) + + it('rejects missing tool', () => { + expect(() => permissionRuleSchema.parse({ effect: 'DENY' })).toThrow() + }) + + it('rejects empty tool string', () => { + expect(() => permissionRuleSchema.parse({ effect: 'DENY', tool: '' })).toThrow() + }) + + it('rejects non-string pattern', () => { + expect(() => permissionRuleSchema.parse({ effect: 'DENY', tool: 'read_file', pattern: 123 })).toThrow() + }) + + it('rejects unknown extra fields', () => { + expect(() => + permissionRuleSchema.parse({ effect: 'DENY', tool: 'read_file', pattern: '*', unknown: 'x' }), + ).toThrow() + }) +}) + +describe('permissionConfigSchema', () => { + it('accepts a config with version and rules', () => { + const config = { + version: 1, + rules: [ + { effect: 'DENY', tool: 'run_command', pattern: 'rm -rf *' }, + { effect: 'ALLOW', tool: 'read_file', pattern: '/ubiquity/**' }, + ], + } + expect(permissionConfigSchema.parse(config)).toEqual(config) + }) + + it('accepts a config with empty rules array', () => { + const config = { version: 1, rules: [] } + expect(permissionConfigSchema.parse(config)).toEqual(config) + }) + + it('rejects missing version', () => { + expect(() => permissionConfigSchema.parse({ rules: [] })).toThrow() + }) + + it('rejects unknown version', () => { + expect(() => permissionConfigSchema.parse({ version: 2, rules: [] })).toThrow() + }) + + it('rejects unknown extra fields at top level', () => { + expect(() => permissionConfigSchema.parse({ version: 1, rules: [], extra: 'x' })).toThrow() + }) +}) + +describe('PermissionEffect type', () => { + it('has exactly 3 values: ALLOW, DENY, ASK', () => { + const effects: PermissionEffect[] = ['ALLOW', 'DENY', 'ASK'] + expect(effects).toHaveLength(3) + expect(effects).not.toContain('ALWAYS') + }) +}) diff --git a/src/server/permissions/schema.ts b/src/server/permissions/schema.ts new file mode 100644 index 00000000..af6b6d0c --- /dev/null +++ b/src/server/permissions/schema.ts @@ -0,0 +1,45 @@ +import { z } from 'zod' + +export type { PermissionEffect, PermissionRule, PermissionConfig } from '../../shared/permissions.js' + +export const permissionEffectSchema = z.enum(['ALLOW', 'DENY', 'ASK']) + +const PATTERN_TOOLS = new Set(['read_file', 'write_file', 'edit_file', 'run_command']) + +export const permissionRuleSchema = z + .object({ + effect: permissionEffectSchema, + tool: z.string().min(1), + pattern: z.string().optional(), + description: z.string().optional(), + }) + .strict() + .superRefine((rule, ctx) => { + if (!PATTERN_TOOLS.has(rule.tool)) { + if (rule.effect !== 'DENY') { + ctx.addIssue({ + code: 'custom', + message: `Tool "${rule.tool}" only supports DENY rules (no path/command target to match patterns against)`, + path: ['effect'], + }) + } + if (rule.pattern !== undefined) { + ctx.addIssue({ + code: 'custom', + message: `Tool "${rule.tool}" does not support patterns (no path/command target)`, + path: ['pattern'], + }) + } + } + }) + +export const permissionConfigSchema = z + .object({ + version: z.literal(1), + rules: z.array(permissionRuleSchema), + }) + .strict() + +import type { PermissionConfig } from '../../shared/permissions.js' + +export const EMPTY_CONFIG: PermissionConfig = { version: 1, rules: [] } diff --git a/src/server/routes/permissions.test.ts b/src/server/routes/permissions.test.ts new file mode 100644 index 00000000..ee9782fa --- /dev/null +++ b/src/server/routes/permissions.test.ts @@ -0,0 +1,135 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' +import express from 'express' +import { mkdir, rm, writeFile, stat, readFile } from 'node:fs/promises' +import { join } from 'node:path' +import { tmpdir } from 'node:os' +import { createPermissionsRoutes } from './permissions.js' + +const TEST_DIR = join(tmpdir(), 'openfox-permissions-route-test') +const GLOBAL_DIR = join(TEST_DIR, 'global') +const PROJECT_DIR = join(TEST_DIR, 'project') + +vi.mock('../utils/logger.js', () => ({ + logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() }, +})) + +let app: express.Express +let server: ReturnType +let baseUrl: string + +beforeEach(async () => { + await rm(TEST_DIR, { recursive: true, force: true }) + await mkdir(GLOBAL_DIR, { recursive: true }) + await mkdir(PROJECT_DIR, { recursive: true }) + await mkdir(join(PROJECT_DIR, '.openfox'), { recursive: true }) + app = express() + app.use(express.json()) + app.use('/api/permissions', createPermissionsRoutes(GLOBAL_DIR)) + await new Promise((resolve) => { + server = app.listen(0, () => { + const addr = server.address() + if (addr && typeof addr === 'object') { + baseUrl = `http://127.0.0.1:${addr.port}` + resolve() + } + }) + }) +}) + +afterEach(async () => { + await new Promise((resolve) => server.close(() => resolve())) + await rm(TEST_DIR, { recursive: true, force: true }) +}) + +describe('GET /api/permissions', () => { + it('returns empty config when no file exists', async () => { + const res = await fetch(`${baseUrl}/api/permissions?scope=global`) + expect(res.status).toBe(200) + const data = (await res.json()) as { config: unknown } + expect(data).toEqual({ config: { version: 1, rules: [] } }) + }) + + it('returns config when file exists', async () => { + const config = { version: 1, rules: [{ effect: 'DENY', tool: 'run_command', pattern: 'rm -rf *' }] } + await writeFile(join(GLOBAL_DIR, 'permissions.json'), JSON.stringify(config)) + const res = await fetch(`${baseUrl}/api/permissions?scope=global`) + expect(res.status).toBe(200) + const data = (await res.json()) as { config: unknown } + expect(data.config).toEqual(config) + }) + + it('returns project config from .openfox/permissions.json', async () => { + const config = { version: 1, rules: [{ effect: 'ALLOW', tool: 'read_file', pattern: '/x/**' }] } + await writeFile(join(PROJECT_DIR, '.openfox', 'permissions.json'), JSON.stringify(config)) + const res = await fetch(`${baseUrl}/api/permissions?scope=project&workdir=${PROJECT_DIR}`) + expect(res.status).toBe(200) + const data = (await res.json()) as { config: unknown } + expect(data.config).toEqual(config) + }) + + it('rejects missing scope', async () => { + const res = await fetch(`${baseUrl}/api/permissions`) + expect(res.status).toBe(400) + }) + + it('rejects invalid scope', async () => { + const res = await fetch(`${baseUrl}/api/permissions?scope=invalid`) + expect(res.status).toBe(400) + }) + + it('rejects project scope without workdir', async () => { + const res = await fetch(`${baseUrl}/api/permissions?scope=project`) + expect(res.status).toBe(400) + }) +}) + +describe('POST /api/permissions', () => { + it('saves config and returns it', async () => { + const config = { + version: 1, + rules: [{ effect: 'DENY', tool: 'run_command', pattern: 'rm -rf *' }], + } + const res = await fetch(`${baseUrl}/api/permissions?scope=global`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(config), + }) + expect(res.status).toBe(200) + const data = (await res.json()) as { config: unknown } + expect(data.config).toEqual(config) + const saved = JSON.parse(await readFile(join(GLOBAL_DIR, 'permissions.json'), 'utf-8')) + expect(saved).toEqual(config) + }) + + it('deletes file when rules empty', async () => { + await writeFile( + join(GLOBAL_DIR, 'permissions.json'), + JSON.stringify({ version: 1, rules: [{ effect: 'DENY', tool: 'x' }] }), + ) + const res = await fetch(`${baseUrl}/api/permissions?scope=global`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ version: 1, rules: [] }), + }) + expect(res.status).toBe(200) + await expect(stat(join(GLOBAL_DIR, 'permissions.json'))).rejects.toThrow() + }) + + it('rejects invalid config (bad effect)', async () => { + const res = await fetch(`${baseUrl}/api/permissions?scope=global`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ version: 1, rules: [{ effect: 'BAD', tool: 'x' }] }), + }) + expect(res.status).toBe(400) + }) + + it('rejects missing scope', async () => { + const res = await fetch(`${baseUrl}/api/permissions`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ version: 1, rules: [] }), + }) + expect(res.status).toBe(400) + }) +}) diff --git a/src/server/routes/permissions.ts b/src/server/routes/permissions.ts new file mode 100644 index 00000000..c4931a93 --- /dev/null +++ b/src/server/routes/permissions.ts @@ -0,0 +1,45 @@ +import { Router } from 'express' +import { loadPermissionsConfig, savePermissionsConfig, type PermissionsScope } from '../permissions/registry.js' +import { permissionConfigSchema } from '../permissions/schema.js' + +export function createPermissionsRoutes(configDir: string): Router { + const router = Router() + + function parseScope(req: { + query: Record + }): { scope: PermissionsScope; workdir: string } | { error: string } { + const scope = req.query['scope'] as string + if (scope !== 'global' && scope !== 'project') { + return { error: 'scope must be "global" or "project"' } + } + const workdir = (req.query['workdir'] as string) ?? '' + if (scope === 'project' && !workdir) { + return { error: 'workdir required for project scope' } + } + return { scope: scope as PermissionsScope, workdir } + } + + router.get('/', async (req, res) => { + const parsed = parseScope(req) + if ('error' in parsed) return res.status(400).json({ error: parsed.error }) + const config = await loadPermissionsConfig(parsed.scope, configDir, parsed.workdir) + res.json({ config }) + }) + + router.post('/', async (req, res) => { + const parsed = parseScope(req) + if ('error' in parsed) return res.status(400).json({ error: parsed.error }) + const parseResult = permissionConfigSchema.safeParse(req.body) + if (!parseResult.success) { + return res.status(400).json({ error: 'Invalid config', issues: parseResult.error.issues }) + } + try { + await savePermissionsConfig(parsed.scope, configDir, parsed.workdir, parseResult.data) + res.json({ config: parseResult.data }) + } catch (err) { + res.status(500).json({ error: err instanceof Error ? err.message : 'Failed to save permissions config' }) + } + }) + + return router +} diff --git a/src/server/tools/index.ts b/src/server/tools/index.ts index 3963099c..83a458da 100644 --- a/src/server/tools/index.ts +++ b/src/server/tools/index.ts @@ -437,6 +437,7 @@ export { cancelPathConfirmationsForSession, providePathConfirmation, getConfirmationSessionId, + clearAllowedPaths, } from './path-security.js' export { stepDoneTool } from './step-done.js' export { setTasksService } from './project-tasks.js' diff --git a/src/server/tools/path-security.rules.test.ts b/src/server/tools/path-security.rules.test.ts new file mode 100644 index 00000000..9005659d --- /dev/null +++ b/src/server/tools/path-security.rules.test.ts @@ -0,0 +1,613 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' +import { join } from 'node:path' +import { tmpdir } from 'node:os' +import { + requestPathAccess, + PathAccessDeniedError, + providePathConfirmation, + hasPendingPathConfirmation, + clearAllowedPaths, + cancelPathConfirmationsForSession, + getSessionAllowedRules, +} from './path-security.js' +import type { PermissionRule } from '../permissions/schema.js' + +vi.mock('../utils/platform.js', () => ({ + getPlatformShell: vi.fn(() => ({ command: '/bin/sh', args: ['-c'] })), +})) + +const TEST_DIR = join(tmpdir(), 'openfox-path-security-rules-test') +const WORKDIR = join(TEST_DIR, 'workdir') +const OUTSIDE = '/var/lib/openfox-rules-test' + +const noOpEvent = vi.fn() + +function rule(effect: PermissionRule['effect'], tool: string, pattern?: string): PermissionRule { + return { effect, tool, ...(pattern !== undefined ? { pattern } : {}) } +} + +beforeEach(() => { + noOpEvent.mockClear() + clearAllowedPaths('rules-session') + cancelPathConfirmationsForSession('rules-session', 'cleanup') +}) + +afterEach(() => { + cancelPathConfirmationsForSession('rules-session', 'cleanup') + clearAllowedPaths('rules-session') +}) + +describe('requestPathAccess with permission rules', () => { + it('DENY rule on read_file throws PathAccessDeniedError, no prompt emitted', async () => { + const rules = [rule('DENY', 'read_file', '/secret/**')] + await expect( + requestPathAccess( + ['/secret/key.pem'], + WORKDIR, + 'rules-session', + 'c1', + 'read_file', + noOpEvent, + 'normal', + undefined, + false, + rules, + ), + ).rejects.toMatchObject({ + name: 'PathAccessDeniedError', + reason: 'rule_denied', + }) + expect(noOpEvent).not.toHaveBeenCalled() + expect(hasPendingPathConfirmation('c1')).toBe(false) + }) + + it('ALLOW rule on read_file for outside-workdir path: no prompt, allowed', async () => { + const rules = [rule('ALLOW', 'read_file', '/ubiquity/**')] + await expect( + requestPathAccess( + ['/ubiquity/deploy.yaml'], + WORKDIR, + 'rules-session', + 'c2', + 'read_file', + noOpEvent, + 'normal', + undefined, + false, + rules, + ), + ).resolves.toBeUndefined() + expect(noOpEvent).not.toHaveBeenCalled() + }) + + it('ASK rule on write_file forces prompt even for path inside workdir', async () => { + const target = join(WORKDIR, '.env') + const rules = [rule('ASK', 'write_file', '**/.env*')] + const promise = requestPathAccess( + [target], + WORKDIR, + 'rules-session', + 'c3', + 'write_file', + noOpEvent, + 'normal', + undefined, + false, + rules, + ) + for (let i = 0; i < 20 && !hasPendingPathConfirmation('c3'); i++) { + await new Promise((r) => setTimeout(r, 5)) + } + expect(hasPendingPathConfirmation('c3')).toBe(true) + providePathConfirmation('c3', false) + await expect(promise).rejects.toMatchObject({ name: 'PathAccessDeniedError' }) + }) + + it('DENY rule on run_command blocks command before execution, no prompt', async () => { + const rules = [rule('DENY', 'run_command', 'rm -rf *')] + await expect( + requestPathAccess( + [], + WORKDIR, + 'rules-session', + 'c4', + 'run_command', + noOpEvent, + 'normal', + 'rm -rf /home/x', + false, + rules, + ), + ).rejects.toMatchObject({ + name: 'PathAccessDeniedError', + reason: 'rule_denied', + }) + expect(noOpEvent).not.toHaveBeenCalled() + }) + + it('no rules → exact same behavior as today (regression: outside path prompts)', async () => { + const promise = requestPathAccess( + [OUTSIDE], + WORKDIR, + 'rules-session', + 'c5', + 'read_file', + noOpEvent, + 'normal', + undefined, + false, + undefined, + ) + for (let i = 0; i < 20 && !hasPendingPathConfirmation('c5'); i++) { + await new Promise((r) => setTimeout(r, 5)) + } + expect(hasPendingPathConfirmation('c5')).toBe(true) + providePathConfirmation('c5', false) + await expect(promise).rejects.toMatchObject({ name: 'PathAccessDeniedError' }) + }) + + it('DENY rule wins even in dangerous mode (explicit guardrail)', async () => { + const rules = [rule('DENY', 'read_file', '/secret/**')] + await expect( + requestPathAccess( + ['/secret/key.pem'], + WORKDIR, + 'rules-session', + 'c6', + 'read_file', + noOpEvent, + 'dangerous', + undefined, + false, + rules, + ), + ).rejects.toMatchObject({ + name: 'PathAccessDeniedError', + reason: 'rule_denied', + }) + }) + + it('ALLOW rule in dangerous mode: allowed, no prompt', async () => { + const rules = [rule('ALLOW', 'read_file', '/ubiquity/**')] + await expect( + requestPathAccess( + ['/ubiquity/a.yaml'], + WORKDIR, + 'rules-session', + 'c7', + 'read_file', + noOpEvent, + 'dangerous', + undefined, + false, + rules, + ), + ).resolves.toBeUndefined() + expect(noOpEvent).not.toHaveBeenCalled() + }) + + it('ASK rule in dangerous mode: dangerous bypasses ASK (no prompt)', async () => { + const target = join(WORKDIR, '.env') + const rules = [rule('ASK', 'write_file', '**/.env*')] + await expect( + requestPathAccess( + [target], + WORKDIR, + 'rules-session', + 'c8', + 'write_file', + noOpEvent, + 'dangerous', + undefined, + false, + rules, + ), + ).resolves.toBeUndefined() + expect(noOpEvent).not.toHaveBeenCalled() + }) + + it('rule_denied error has descriptive message', async () => { + const rules = [rule('DENY', 'run_command', 'rm -rf *')] + try { + await requestPathAccess( + [], + WORKDIR, + 'rules-session', + 'c9', + 'run_command', + noOpEvent, + 'normal', + 'rm -rf /x', + false, + rules, + ) + expect.fail('should have thrown') + } catch (err) { + expect(err).toBeInstanceOf(PathAccessDeniedError) + expect((err as PathAccessDeniedError).message).toContain('rm -rf /x') + expect((err as PathAccessDeniedError).reason).toBe('rule_denied') + } + }) + + it('DENY on second path wins over ALLOW on first path (cross-target precedence)', async () => { + const rules = [rule('ALLOW', 'read_file', '/workdir/**'), rule('DENY', 'read_file', '/secret/**')] + await expect( + requestPathAccess( + ['/workdir/a.txt', '/secret/key.pem'], + WORKDIR, + 'rules-session', + 'c10', + 'read_file', + noOpEvent, + 'normal', + undefined, + false, + rules, + ), + ).rejects.toMatchObject({ + name: 'PathAccessDeniedError', + reason: 'rule_denied', + }) + }) + + it('ALLOW rule does NOT bypass git --no-verify confirmation (always-confirm guard)', async () => { + const rules = [rule('ALLOW', 'run_command', 'git *')] + const promise = requestPathAccess( + [], + WORKDIR, + 'rules-session', + 'c11', + 'run_command', + noOpEvent, + 'normal', + 'git commit --no-verify -m "skip"', + false, + rules, + ) + for (let i = 0; i < 20 && !hasPendingPathConfirmation('c11'); i++) { + await new Promise((r) => setTimeout(r, 5)) + } + expect(hasPendingPathConfirmation('c11')).toBe(true) + providePathConfirmation('c11', false) + await expect(promise).rejects.toMatchObject({ name: 'PathAccessDeniedError' }) + }) + + it('ALLOW rule does NOT bypass dangerous-command confirmation (always-confirm guard)', async () => { + const rules = [rule('ALLOW', 'run_command', 'rm *')] + const promise = requestPathAccess( + [], + WORKDIR, + 'rules-session', + 'c12', + 'run_command', + noOpEvent, + 'normal', + 'rm -rf ~', + false, + rules, + ) + for (let i = 0; i < 20 && !hasPendingPathConfirmation('c12'); i++) { + await new Promise((r) => setTimeout(r, 5)) + } + expect(hasPendingPathConfirmation('c12')).toBe(true) + providePathConfirmation('c12', false) + await expect(promise).rejects.toMatchObject({ name: 'PathAccessDeniedError' }) + }) + + it('ASK rule with sub-agent: fails closed (no bypass via sub-agent shortcut)', async () => { + const target = join(WORKDIR, '.env') + const rules = [rule('ASK', 'write_file', '**/.env*')] + await expect( + requestPathAccess( + [target], + WORKDIR, + 'rules-session', + 'c13', + 'write_file', + noOpEvent, + 'normal', + undefined, + true, + rules, + ), + ).rejects.toMatchObject({ name: 'PathAccessDeniedError', reason: 'rule_ask' }) + expect(noOpEvent).not.toHaveBeenCalled() + }) + + it('ASK rule with sub-agent in dangerous mode: bypassed (dangerous overrides ASK)', async () => { + const target = join(WORKDIR, '.env') + const rules = [rule('ASK', 'write_file', '**/.env*')] + await expect( + requestPathAccess( + [target], + WORKDIR, + 'rules-session', + 'c14', + 'write_file', + noOpEvent, + 'dangerous', + undefined, + true, + rules, + ), + ).resolves.toBeUndefined() + }) +}) + +describe('requestPathAccess: Allow for this session (rule ASK → ALLOW ephemeral)', () => { + it('ASK rule → allow for session → 2nd call same pattern: no prompt', async () => { + const rules = [rule('ASK', 'run_command', 'terragrunt destroy *')] + const cmd = 'terragrunt destroy -auto-approve' + const callId1 = 's1' + const promise1 = requestPathAccess( + [], + WORKDIR, + 'rules-session', + callId1, + 'run_command', + noOpEvent, + 'normal', + cmd, + false, + rules, + ) + for (let i = 0; i < 20 && !hasPendingPathConfirmation(callId1); i++) { + await new Promise((r) => setTimeout(r, 5)) + } + expect(hasPendingPathConfirmation(callId1)).toBe(true) + providePathConfirmation(callId1, true, true) + await expect(promise1).resolves.toBeUndefined() + + noOpEvent.mockClear() + const sessionRules = [...rules, ...getSessionAllowedRules('rules-session')] + const callId2 = 's2' + await expect( + requestPathAccess( + [], + WORKDIR, + 'rules-session', + callId2, + 'run_command', + noOpEvent, + 'normal', + cmd, + false, + sessionRules, + ), + ).resolves.toBeUndefined() + expect(noOpEvent).not.toHaveBeenCalled() + }) + + it('ASK rule → deny → 2nd call: re-prompts (no clone)', async () => { + const rules = [rule('ASK', 'run_command', 'terragrunt destroy *')] + const cmd = 'terragrunt destroy -auto-approve' + const callId1 = 'd1' + const promise1 = requestPathAccess( + [], + WORKDIR, + 'rules-session', + callId1, + 'run_command', + noOpEvent, + 'normal', + cmd, + false, + rules, + ) + for (let i = 0; i < 20 && !hasPendingPathConfirmation(callId1); i++) { + await new Promise((r) => setTimeout(r, 5)) + } + expect(hasPendingPathConfirmation(callId1)).toBe(true) + providePathConfirmation(callId1, false) + await expect(promise1).rejects.toMatchObject({ name: 'PathAccessDeniedError', reason: 'rule_ask' }) + + expect(getSessionAllowedRules('rules-session')).toHaveLength(0) + }) + + it('ASK rule → allow one-shot (alwaysAllow=false) → 2nd call: re-prompts', async () => { + const rules = [rule('ASK', 'run_command', 'terragrunt destroy *')] + const cmd = 'terragrunt destroy -auto-approve' + const callId1 = 'o1' + const promise1 = requestPathAccess( + [], + WORKDIR, + 'rules-session', + callId1, + 'run_command', + noOpEvent, + 'normal', + cmd, + false, + rules, + ) + for (let i = 0; i < 20 && !hasPendingPathConfirmation(callId1); i++) { + await new Promise((r) => setTimeout(r, 5)) + } + expect(hasPendingPathConfirmation(callId1)).toBe(true) + providePathConfirmation(callId1, true, false) + await expect(promise1).resolves.toBeUndefined() + + expect(getSessionAllowedRules('rules-session')).toHaveLength(0) + }) + + it('DENY disk rule + ALLOW ephemeral session rule on same pattern: DENY wins (3>2)', async () => { + const denyRule = rule('DENY', 'run_command', 'terragrunt destroy *') + const askRule = rule('ASK', 'run_command', 'terragrunt destroy *') + const cmd = 'terragrunt destroy -auto-approve' + const callId1 = 'p1' + const promise1 = requestPathAccess( + [], + WORKDIR, + 'rules-session', + callId1, + 'run_command', + noOpEvent, + 'normal', + cmd, + false, + [askRule], + ) + for (let i = 0; i < 20 && !hasPendingPathConfirmation(callId1); i++) { + await new Promise((r) => setTimeout(r, 5)) + } + expect(hasPendingPathConfirmation(callId1)).toBe(true) + providePathConfirmation(callId1, true, true) + await expect(promise1).resolves.toBeUndefined() + + const sessionRules = [denyRule, ...getSessionAllowedRules('rules-session')] + const callId2 = 'p2' + await expect( + requestPathAccess( + [], + WORKDIR, + 'rules-session', + callId2, + 'run_command', + noOpEvent, + 'normal', + cmd, + false, + sessionRules, + ), + ).rejects.toMatchObject({ name: 'PathAccessDeniedError', reason: 'rule_denied' }) + }) + + it('clearAllowedRules → 2nd call re-prompts', async () => { + const rules = [rule('ASK', 'run_command', 'terragrunt destroy *')] + const cmd = 'terragrunt destroy -auto-approve' + const callId1 = 'cl1' + const promise1 = requestPathAccess( + [], + WORKDIR, + 'rules-session', + callId1, + 'run_command', + noOpEvent, + 'normal', + cmd, + false, + rules, + ) + for (let i = 0; i < 20 && !hasPendingPathConfirmation(callId1); i++) { + await new Promise((r) => setTimeout(r, 5)) + } + expect(hasPendingPathConfirmation(callId1)).toBe(true) + providePathConfirmation(callId1, true, true) + await expect(promise1).resolves.toBeUndefined() + + clearAllowedPaths('rules-session') + expect(getSessionAllowedRules('rules-session')).toHaveLength(0) + + const callId2 = 'cl2' + const promise2 = requestPathAccess( + [], + WORKDIR, + 'rules-session', + callId2, + 'run_command', + noOpEvent, + 'normal', + cmd, + false, + rules, + ) + for (let i = 0; i < 20 && !hasPendingPathConfirmation(callId2); i++) { + await new Promise((r) => setTimeout(r, 5)) + } + expect(hasPendingPathConfirmation(callId2)).toBe(true) + providePathConfirmation(callId2, false) + await expect(promise2).rejects.toMatchObject({ name: 'PathAccessDeniedError', reason: 'rule_ask' }) + }) + + it('ASK rule on read_file path → allow for session → 2nd read same path: no prompt', async () => { + const rules = [rule('ASK', 'read_file', '/tmp/**')] + const target = '/tmp/foo.txt' + const callId1 = 'rp1' + const promise1 = requestPathAccess( + [target], + WORKDIR, + 'rules-session', + callId1, + 'read_file', + noOpEvent, + 'normal', + undefined, + false, + rules, + ) + for (let i = 0; i < 20 && !hasPendingPathConfirmation(callId1); i++) { + await new Promise((r) => setTimeout(r, 5)) + } + expect(hasPendingPathConfirmation(callId1)).toBe(true) + providePathConfirmation(callId1, true, true) + await expect(promise1).resolves.toBeUndefined() + + noOpEvent.mockClear() + const sessionRules = [...rules, ...getSessionAllowedRules('rules-session')] + const callId2 = 'rp2' + await expect( + requestPathAccess( + [target], + WORKDIR, + 'rules-session', + callId2, + 'read_file', + noOpEvent, + 'normal', + undefined, + false, + sessionRules, + ), + ).resolves.toBeUndefined() + expect(noOpEvent).not.toHaveBeenCalled() + }) + + it('clearAllowedPaths purges session-allowed rules (ephemeral cleanup on session delete)', async () => { + const rules = [rule('ASK', 'run_command', 'terragrunt destroy *')] + const cmd = 'terragrunt destroy /tmp/x' + const callId = 'purge-1' + const promise = requestPathAccess( + [], + WORKDIR, + 'rules-session', + callId, + 'run_command', + noOpEvent, + 'normal', + cmd, + false, + rules, + ) + for (let i = 0; i < 20 && !hasPendingPathConfirmation(callId); i++) { + await new Promise((r) => setTimeout(r, 5)) + } + providePathConfirmation(callId, true, true) + await expect(promise).resolves.toBeUndefined() + expect(getSessionAllowedRules('rules-session')).toHaveLength(1) + + clearAllowedPaths('rules-session') + + expect(getSessionAllowedRules('rules-session')).toHaveLength(0) + + // After purge, the same command re-prompts (no lingering ALLOW) + const callId2 = 'purge-2' + const promise2 = requestPathAccess( + [], + WORKDIR, + 'rules-session', + callId2, + 'run_command', + noOpEvent, + 'normal', + cmd, + false, + rules, + ) + for (let i = 0; i < 20 && !hasPendingPathConfirmation(callId2); i++) { + await new Promise((r) => setTimeout(r, 5)) + } + expect(hasPendingPathConfirmation(callId2)).toBe(true) + providePathConfirmation(callId2, false) + await expect(promise2).rejects.toMatchObject({ name: 'PathAccessDeniedError', reason: 'rule_ask' }) + }) +}) diff --git a/src/server/tools/path-security.ts b/src/server/tools/path-security.ts index d406f13f..8efc018c 100644 --- a/src/server/tools/path-security.ts +++ b/src/server/tools/path-security.ts @@ -5,6 +5,8 @@ import type { ServerMessage } from '../../shared/protocol.js' import { createChatPathConfirmationMessage } from '../ws/protocol.js' import { getEventStore } from '../events/index.js' import { getPlatformShell } from '../utils/platform.js' +import type { PermissionRule } from '../permissions/schema.js' +import { evaluateRules, evaluateRulesWithMatch } from '../permissions/rules.js' // =========================================================================== // Constants @@ -75,6 +77,9 @@ export function isSensitivePath(path: string): boolean { /** Per-session set of paths that user has approved for access */ const sessionAllowedPaths = new Map>() +/** Per-session set of ALLOW rules promoted from ASK rules (Allow for this session) */ +const sessionAllowedRules = new Map() + /** * Add a path to the session's allowlist (user approved it) */ @@ -104,10 +109,32 @@ export function isPathAllowed(sessionId: string, path: string): boolean { } /** - * Clear the session's allowlist (e.g., on session delete) + * Add a session-scoped ALLOW rule (promoted from an ASK rule via "Allow for this session"). + * The rule is ephemeral — lives only for the session, not persisted to disk. + */ +export function addSessionAllowedRule(sessionId: string, rule: PermissionRule): void { + const existing = sessionAllowedRules.get(sessionId) + if (existing) { + existing.push(rule) + } else { + sessionAllowedRules.set(sessionId, [rule]) + } +} + +/** + * Get the session-scoped ALLOW rules for a session. + * Returns an empty array if none. + */ +export function getSessionAllowedRules(sessionId: string): PermissionRule[] { + return sessionAllowedRules.get(sessionId) ?? [] +} + +/** + * Clear the session's allowlist and session-scoped rules (e.g., on session delete) */ export function clearAllowedPaths(sessionId: string): void { sessionAllowedPaths.delete(sessionId) + sessionAllowedRules.delete(sessionId) } // =========================================================================== @@ -885,32 +912,19 @@ export async function requestPathAccess( dangerLevel?: string, command?: string, isSubAgent?: boolean, + rules?: PermissionRule[], ): Promise { - // Sub-agent shortcut: skip all confirmation dialogs since they don't render - // properly in the small sub-agent window. Fail closed in normal mode; - // auto-approve everything in dangerous mode. - if (isSubAgent) { - const result = await checkPathsAccess(paths, workdir, sessionId) - if (!result.needsConfirmation) return - - if (dangerLevel === 'dangerous') { - const allPaths = [...new Set([...result.deniedPaths, ...result.sensitivePaths])] - addAllowedPaths(sessionId, allPaths) - return - } - - const allPaths = [...new Set([...result.deniedPaths, ...result.sensitivePaths])] - const hasDenied = result.deniedPaths.length > 0 - const hasSensitive = result.sensitivePaths.length > 0 - const reason: PathDenialReason = - hasDenied && hasSensitive ? 'both' : hasDenied ? 'outside_workdir' : 'sensitive_file' - throw new PathAccessDeniedError(allPaths, tool, reason) - } - // Helper to emit path.confirmation_pending event const emitPendingEvent = ( confirmationPaths: string[], - confirmationReason: 'outside_workdir' | 'sensitive_file' | 'both' | 'dangerous_command' | 'git_no_verify', + confirmationReason: + | 'outside_workdir' + | 'sensitive_file' + | 'both' + | 'dangerous_command' + | 'git_no_verify' + | 'rule_denied' + | 'rule_ask', ) => { try { const eventStore = getEventStore() @@ -923,9 +937,11 @@ export async function requestPathAccess( } } - // Check for git --no-verify - ALWAYS requires confirmation, even in dangerous mode - // This ensures the user is aware the agent is bypassing hooks/pre-commit checks - if (command && extractGitNoVerify(command)) { + // git --no-verify ALWAYS requires confirmation, even in dangerous mode and + // even when an ALLOW permission rule matches. This is a hardcoded safety + // guard that permission rules cannot override. Skipped for sub-agents (they + // don't render confirmation dialogs — handled by the sub-agent shortcut below). + if (!isSubAgent && command && extractGitNoVerify(command)) { emitPendingEvent([workdir], 'git_no_verify') const confirmationPromise = registerPathConfirmation(callId, [workdir], sessionId, tool, workdir, 'git_no_verify') onEvent(createChatPathConfirmationMessage(callId, tool, ['git --no-verify detected'], workdir, 'git_no_verify')) @@ -940,8 +956,10 @@ export async function requestPathAccess( } } - // Check for dangerous commands that need confirmation even without path access - if (dangerLevel !== 'dangerous' && command) { + // Dangerous command patterns ALWAYS require confirmation in normal mode, + // even when an ALLOW permission rule matches. Permission rules cannot + // override these hardcoded safety guards. Skipped for sub-agents. + if (!isSubAgent && dangerLevel !== 'dangerous' && command) { const dangerousPatterns = extractDangerousPatterns(command) if (dangerousPatterns.length > 0) { emitPendingEvent([workdir], 'dangerous_command') @@ -963,6 +981,82 @@ export async function requestPathAccess( } } + // Permission rules evaluation: DENY > ALLOW > ASK across ALL targets in the + // call (command + paths). We scan all targets for DENY first, then ALLOW, + // then ASK — so a DENY on any target always wins over an ALLOW on another. + if (rules && rules.length > 0) { + const allTargets: string[] = [...paths] + if (command) allTargets.unshift(command) + + // Phase 1: DENY — if ANY target matches DENY, throw immediately + for (const target of allTargets) { + const effect = evaluateRules(rules, tool, target) + if (effect === 'DENY') { + throw new PathAccessDeniedError([target], tool, 'rule_denied', `Permission rule DENY blocked: "${target}"`) + } + } + + // Phase 2: ALLOW — if ANY target matches ALLOW, skip sandbox/sensitive checks + const hasAllow = allTargets.some((t) => evaluateRules(rules, tool, t) === 'ALLOW') + if (hasAllow) return + + // Phase 3: ASK — if ANY target matches ASK, prompt (top-level only). + // Sub-agents can't render dialogs: fail closed in normal mode so the ASK + // rule is respected instead of being silently bypassed by the sub-agent + // shortcut below. In dangerous mode, ASK is skipped (same as top-level). + const askTargets: string[] = [] + let matchedAskRule: PermissionRule | null = null + for (const target of allTargets) { + const match = evaluateRulesWithMatch(rules, tool, target) + if (match.effect === 'ASK') { + askTargets.push(target) + if (match.rule) matchedAskRule = match.rule + } + } + if (askTargets.length > 0 && dangerLevel !== 'dangerous') { + if (isSubAgent) { + throw new PathAccessDeniedError(askTargets, tool, 'rule_ask') + } + emitPendingEvent(askTargets, 'rule_ask') + const confirmationPromise = registerPathConfirmation( + callId, + askTargets, + sessionId, + tool, + workdir, + 'rule_ask', + matchedAskRule ?? undefined, + ) + onEvent(createChatPathConfirmationMessage(callId, tool, askTargets, workdir, 'rule_ask')) + const approved = await confirmationPromise + if (!approved) { + throw new PathAccessDeniedError(askTargets, tool, 'rule_ask') + } + return + } + } + + // Sub-agent shortcut: skip all confirmation dialogs since they don't render + // properly in the small sub-agent window. Fail closed in normal mode; + // auto-approve everything in dangerous mode. + if (isSubAgent) { + const result = await checkPathsAccess(paths, workdir, sessionId) + if (!result.needsConfirmation) return + + if (dangerLevel === 'dangerous') { + const allPaths = [...new Set([...result.deniedPaths, ...result.sensitivePaths])] + addAllowedPaths(sessionId, allPaths) + return + } + + const allPaths = [...new Set([...result.deniedPaths, ...result.sensitivePaths])] + const hasDenied = result.deniedPaths.length > 0 + const hasSensitive = result.sensitivePaths.length > 0 + const reason: PathDenialReason = + hasDenied && hasSensitive ? 'both' : hasDenied ? 'outside_workdir' : 'sensitive_file' + throw new PathAccessDeniedError(allPaths, tool, reason) + } + // Check which paths need confirmation const result = await checkPathsAccess(paths, workdir, sessionId) @@ -1021,7 +1115,8 @@ export async function requestPathAccess( // Error Classes // =========================================================================== -export type PathDenialReason = 'outside_workdir' | 'sensitive_file' | 'both' | 'dangerous_command' | 'git_no_verify' +export type PathDenialReason = + 'outside_workdir' | 'sensitive_file' | 'both' | 'dangerous_command' | 'git_no_verify' | 'rule_denied' | 'rule_ask' /** * Error thrown when user denies path access. @@ -1041,7 +1136,11 @@ export class PathAccessDeniedError extends Error { ? 'paths outside workdir and sensitive files' : reason === 'git_no_verify' ? 'git commands with --no-verify' - : 'paths outside workdir' + : reason === 'rule_denied' + ? 'paths/commands blocked by a permission rule' + : reason === 'rule_ask' + ? 'paths/commands requiring confirmation per a permission rule' + : 'paths outside workdir' super(customMessage ?? `User denied access to ${reasonText}: ${paths.join(', ')}`) this.name = 'PathAccessDeniedError' } @@ -1061,7 +1160,9 @@ const pendingConfirmations = new Map< sessionId: string tool: string workdir: string - reason: 'outside_workdir' | 'sensitive_file' | 'both' | 'dangerous_command' | 'git_no_verify' + reason: + 'outside_workdir' | 'sensitive_file' | 'both' | 'dangerous_command' | 'git_no_verify' | 'rule_denied' | 'rule_ask' + matchedRule?: PermissionRule | undefined } >() @@ -1075,10 +1176,21 @@ export function registerPathConfirmation( sessionId: string, tool: string, workdir: string, - reason: 'outside_workdir' | 'sensitive_file' | 'both' | 'dangerous_command' | 'git_no_verify', + reason: + 'outside_workdir' | 'sensitive_file' | 'both' | 'dangerous_command' | 'git_no_verify' | 'rule_denied' | 'rule_ask', + matchedRule?: PermissionRule, ): Promise { return new Promise((resolve, reject) => { - pendingConfirmations.set(callId, { resolve, reject, paths, sessionId, tool, workdir, reason }) + pendingConfirmations.set(callId, { + resolve, + reject, + paths, + sessionId, + tool, + workdir, + reason, + ...(matchedRule ? { matchedRule } : {}), + }) }) } @@ -1125,6 +1237,16 @@ export function providePathConfirmation( if (pending.reason !== 'dangerous_command' && pending.reason !== 'git_no_verify') { addAllowedPaths(pending.sessionId, pending.paths) } + // For rule_ask confirmations with alwaysAllow, promote the matched ASK rule + // to a session-scoped ALLOW rule so the same pattern won't re-prompt. + if (pending.reason === 'rule_ask' && pending.matchedRule) { + const promoted: PermissionRule = { + effect: 'ALLOW', + tool: pending.matchedRule.tool, + ...(pending.matchedRule.pattern !== undefined ? { pattern: pending.matchedRule.pattern } : {}), + } + addSessionAllowedRule(pending.sessionId, promoted) + } } pending.resolve(approved) @@ -1198,7 +1320,8 @@ export function getPendingConfirmationsBySession(): Record< tool: string paths: string[] workdir: string - reason: 'outside_workdir' | 'sensitive_file' | 'both' | 'dangerous_command' | 'git_no_verify' + reason: + 'outside_workdir' | 'sensitive_file' | 'both' | 'dangerous_command' | 'git_no_verify' | 'rule_denied' | 'rule_ask' }> > { const bySession: Record< @@ -1208,7 +1331,14 @@ export function getPendingConfirmationsBySession(): Record< tool: string paths: string[] workdir: string - reason: 'outside_workdir' | 'sensitive_file' | 'both' | 'dangerous_command' | 'git_no_verify' + reason: + | 'outside_workdir' + | 'sensitive_file' + | 'both' + | 'dangerous_command' + | 'git_no_verify' + | 'rule_denied' + | 'rule_ask' }> > = {} for (const [_callId, pending] of pendingConfirmations.entries()) { diff --git a/src/server/tools/tool-helpers.ts b/src/server/tools/tool-helpers.ts index 93808e6a..37214e5a 100644 --- a/src/server/tools/tool-helpers.ts +++ b/src/server/tools/tool-helpers.ts @@ -228,6 +228,7 @@ export function createTool(name: string, definition: LLMToolDefinition, h context.dangerLevel, command, context.isSubAgent, + context.permissionRules, ) } }, diff --git a/src/server/tools/types.ts b/src/server/tools/types.ts index d71647ba..5202b90a 100644 --- a/src/server/tools/types.ts +++ b/src/server/tools/types.ts @@ -6,6 +6,7 @@ import type { LspManagerInterface } from '../lsp/types.js' import type { SessionManager } from '../session/manager.js' import type { LLMClientWithModel } from '../llm/client.js' import type { ProviderManager } from '../provider-manager.js' +import type { PermissionRule } from '../permissions/schema.js' export interface ToolContext { workdir: string @@ -23,6 +24,7 @@ export interface ToolContext { permittedActions?: Record | undefined // Map of tool name -> allowed actions (e.g., { criterion: ['pass', 'fail'] }) toolCallId?: string // ID of the tool call being executed (for matching confirmations) agentTimeout?: number // User-configured max tool timeout from config.agent.toolTimeout + permissionRules?: PermissionRule[] // Merged permission rules (global + project) evaluated before sandbox checks } export interface Tool { diff --git a/src/server/ws/protocol.ts b/src/server/ws/protocol.ts index b47e5440..4fdfc994 100644 --- a/src/server/ws/protocol.ts +++ b/src/server/ws/protocol.ts @@ -28,7 +28,6 @@ import type { ChatStepRetryPayload, ChatPathConfirmationPayload, ChatAskUserPayload, - PathConfirmPayload, ModeChangedPayload, PhaseChangedPayload, CriteriaUpdatedPayload, @@ -384,11 +383,6 @@ export function isSessionLoadPayload(payload: unknown): payload is SessionLoadPa // Chat payloads -// Path confirmation payloads -export function isPathConfirmPayload(payload: unknown): payload is PathConfirmPayload { - return typeof payload === 'object' && payload !== null && 'callId' in payload && 'approved' in payload -} - // Ask user payloads export function isAskAnswerPayload(payload: unknown): payload is AskAnswerPayload { return typeof payload === 'object' && payload !== null && 'callId' in payload && 'answer' in payload diff --git a/src/server/ws/server.test.ts b/src/server/ws/server.test.ts index 7cc23d0f..af1e8b12 100644 --- a/src/server/ws/server.test.ts +++ b/src/server/ws/server.test.ts @@ -860,7 +860,7 @@ describe('createWebSocketServer', () => { await harness.close() }) - it('handles mode.accept, runner.launch, context.compact, and path confirmation', async () => { + it('handles mode.accept, runner.launch, and context.compact', async () => { const sessionState: any = { id: 'session-1', projectId: 'project-1', @@ -938,16 +938,6 @@ describe('createWebSocketServer', () => { type: 'context.state', }) - harness.send({ id: 'path-missing', type: 'path.confirm', payload: { callId: 'call-1', approved: true } }) - expect(await harness.nextMessage((message) => message.id === 'path-missing')).toMatchObject({ - payload: { code: 'DEPRECATED' }, - }) - - harness.send({ id: 'path-ok', type: 'path.confirm', payload: { callId: 'call-2', approved: false } }) - expect(await harness.nextMessage((message) => message.id === 'path-ok')).toMatchObject({ - payload: { code: 'DEPRECATED' }, - }) - await harness.close() }) @@ -983,11 +973,6 @@ describe('createWebSocketServer', () => { payload: { code: 'NO_SESSION' }, }) - harness.send({ id: 'path-none', type: 'path.confirm', payload: { callId: 'x', approved: true } }) - expect(await harness.nextMessage((message) => message.id === 'path-none')).toMatchObject({ - payload: { code: 'DEPRECATED' }, - }) - harness.send({ id: 'sl-ok', type: 'session.load', payload: { sessionId: 'session-1' } }) await harness.nextMessage((message) => message.id === 'sl-ok') @@ -1018,11 +1003,6 @@ describe('createWebSocketServer', () => { payload: { code: 'NO_WORK' }, }) - harness.send({ id: 'path-invalid', type: 'path.confirm', payload: {} }) - expect(await harness.nextMessage((message) => message.id === 'path-invalid')).toMatchObject({ - payload: { code: 'DEPRECATED' }, - }) - await harness.close() }) diff --git a/src/server/ws/server.ts b/src/server/ws/server.ts index c1619186..21b8d27e 100644 --- a/src/server/ws/server.ts +++ b/src/server/ws/server.ts @@ -1394,20 +1394,9 @@ async function handleClientMessage( } // ========================================================================= - // Path Confirmation + // Ask User // ========================================================================= - case 'path.confirm': { - send( - createErrorMessage( - 'DEPRECATED', - 'path.confirm removed. Use REST API: POST /api/sessions/:id/confirm-path', - message.id, - ), - ) - break - } - // ========================================================================= // Ask User // ========================================================================= diff --git a/src/shared/permissions.ts b/src/shared/permissions.ts new file mode 100644 index 00000000..24fc0bc3 --- /dev/null +++ b/src/shared/permissions.ts @@ -0,0 +1,19 @@ +export type PermissionEffect = 'ALLOW' | 'DENY' | 'ASK' + +export type PermissionScope = 'global' | 'project' + +export interface PermissionRule { + effect: PermissionEffect + tool: string + pattern?: string | undefined + description?: string | undefined +} + +export interface ScopedPermissionRule extends PermissionRule { + scope: PermissionScope +} + +export interface PermissionConfig { + version: 1 + rules: PermissionRule[] +} diff --git a/src/shared/protocol.ts b/src/shared/protocol.ts index 54bc4043..001adf39 100644 --- a/src/shared/protocol.ts +++ b/src/shared/protocol.ts @@ -31,8 +31,6 @@ export type ClientMessageType = | 'runner.launch' // Start the auto-loop runner (build → verify → done) // Workflow | 'workflow.exit' // Exit/cancel a paused workflow - // Path confirmation - | 'path.confirm' // User response to path confirmation request // Ask user | 'ask.answer' // User response to ask_user question @@ -213,7 +211,8 @@ export interface PendingPathConfirmationPayload { tool: string paths: string[] workdir: string - reason: 'outside_workdir' | 'sensitive_file' | 'both' | 'dangerous_command' | 'git_no_verify' + reason: + 'outside_workdir' | 'sensitive_file' | 'both' | 'dangerous_command' | 'git_no_verify' | 'rule_denied' | 'rule_ask' } export interface SessionListPayload { @@ -341,7 +340,7 @@ export interface ChatStepRetryPayload { // Path confirmation payloads export type PathConfirmationReason = - 'outside_workdir' | 'sensitive_file' | 'both' | 'dangerous_command' | 'git_no_verify' + 'outside_workdir' | 'sensitive_file' | 'both' | 'dangerous_command' | 'git_no_verify' | 'rule_denied' | 'rule_ask' export interface ChatPathConfirmationPayload { callId: string @@ -351,13 +350,6 @@ export interface ChatPathConfirmationPayload { reason: PathConfirmationReason // Why confirmation is needed } -// Client payload for path confirmation response -export interface PathConfirmPayload { - callId: string - approved: boolean - alwaysAllow?: boolean // If true, add paths to session allowlist permanently -} - // Ask user payloads export interface ChatAskUserPayload { callId: string diff --git a/web/package-lock.json b/web/package-lock.json index 5a2ffad8..9aa1f77a 100644 --- a/web/package-lock.json +++ b/web/package-lock.json @@ -39,7 +39,7 @@ }, "..": { "name": "openfox", - "version": "2.0.116", + "version": "2.0.117", "hasInstallScript": true, "license": "MIT", "dependencies": { diff --git a/web/src/components/plan/MessageList.tsx b/web/src/components/plan/MessageList.tsx index d69641f7..e43d2f3c 100644 --- a/web/src/components/plan/MessageList.tsx +++ b/web/src/components/plan/MessageList.tsx @@ -231,7 +231,9 @@ export const MessageList = memo(function MessageList({
{error.code}
-
{error.message}
+
+ {error.message} +
void } -type Tab = 'instructions' | 'skills' | 'plugins' | 'notifications' | 'display' | 'keybindings' | 'advanced' | 'tools' +type Tab = + | 'instructions' + | 'skills' + | 'plugins' + | 'notifications' + | 'display' + | 'keybindings' + | 'advanced' + | 'tools' + | 'permissions' export function GlobalSettingsModal({ isOpen, onClose }: GlobalSettingsModalProps) { const [activeTab, setActiveTab] = useState('instructions') @@ -47,6 +57,7 @@ export function GlobalSettingsModal({ isOpen, onClose }: GlobalSettingsModalProp onClick={() => setActiveTab('instructions')} /> setActiveTab('tools')} /> + setActiveTab('permissions')} /> setActiveTab('skills')} /> setActiveTab('plugins')} /> } {activeTab === 'keybindings' && } {activeTab === 'tools' && } + {activeTab === 'permissions' && } {activeTab === 'advanced' && }
diff --git a/web/src/components/settings/ProjectSettingsModal.test.tsx b/web/src/components/settings/ProjectSettingsModal.test.tsx index bf861c13..502ac309 100644 --- a/web/src/components/settings/ProjectSettingsModal.test.tsx +++ b/web/src/components/settings/ProjectSettingsModal.test.tsx @@ -30,6 +30,21 @@ const { mockDefaultAgents, mockUserAgents, mockProjectAgents, mockFetchAgents } mockFetchAgents: vi.fn(async () => undefined), })) +const { mockPermsStore } = vi.hoisted(() => ({ + mockPermsStore: { + projectConfig: null as any, + saving: false, + fetchConfig: vi.fn(async () => undefined), + addRule: vi.fn(async () => undefined), + updateRule: vi.fn(async () => undefined), + deleteRule: vi.fn(async () => undefined), + }, +})) + +vi.mock('../../stores/permissions', () => ({ + usePermissionsStore: (selector?: any) => (selector ? selector(mockPermsStore) : mockPermsStore), +})) + vi.mock('../../stores/agents', () => ({ useAgentsStore: (selector: any) => selector({ @@ -612,3 +627,25 @@ describe('ProjectSettingsModal — rootDir validation (Criterion 0 & 1)', () => expect(mockSaveConfig).toHaveBeenCalled() }) }) + +describe('ProjectSettingsModal — permission rules', () => { + beforeEach(() => { + mockPermsStore.projectConfig = null + mockPermsStore.saving = false + }) + + it('renders Permission Rules section with Add Rule button', async () => { + render() + expect(screen.getByText('Permission Rules')).toBeDefined() + expect(screen.getByText(/Add Rule/i)).toBeDefined() + }) + + it('renders existing project rules from permissions store', async () => { + mockPermsStore.projectConfig = { + version: 1, + rules: [{ effect: 'DENY', tool: 'run_command', pattern: 'terragrunt destroy *' }], + } + render() + expect(screen.getByText('terragrunt destroy *')).toBeDefined() + }) +}) diff --git a/web/src/components/settings/ProjectSettingsModal.tsx b/web/src/components/settings/ProjectSettingsModal.tsx index 826371af..e78cb7fd 100644 --- a/web/src/components/settings/ProjectSettingsModal.tsx +++ b/web/src/components/settings/ProjectSettingsModal.tsx @@ -8,6 +8,8 @@ import { useProjectStore } from '../../stores/project' import { useAgentsStore } from '../../stores/agents' import { useWorkspaceConfigStore, type WorkspaceConfigResponse } from '../../stores/workspace-config' import { useMcpStore } from '../../stores/mcp' +import { usePermissionsStore } from '../../stores/permissions' +import { PermissionsList } from './permissions-shared' import { mcpStatusColor, mcpStatusDot } from '../../lib/mcp-utils' import { wsClient } from '../../lib/ws' import { authFetch } from '../../lib/api' @@ -30,6 +32,15 @@ export function ProjectSettingsModal({ isOpen, onClose, project }: ProjectSettin const userAgents = useAgentsStore((s) => s.userItems) const projectAgents = useAgentsStore((s) => s.projectItems) const fetchAgents = useAgentsStore((s) => s.fetchAgents) + const { + projectConfig, + saving: permsSaving, + fetchConfig: fetchPermsConfig, + addRule, + updateRule, + deleteRule, + } = usePermissionsStore() + const projectRules = (projectConfig?.rules ?? []).map((r) => ({ ...r, scope: 'project' as const })) const topLevelByScope = { builtin: defaultAgents.filter((a) => !a.subagent), user: userAgents.filter((a) => !a.subagent), @@ -101,8 +112,9 @@ export function ProjectSettingsModal({ isOpen, onClose, project }: ProjectSettin setExpandedServers(new Set()) fetchWsConfig(project.workdir) fetchAgents(project.workdir).catch(() => {}) + fetchPermsConfig('project', project.workdir).catch(() => {}) } - }, [isOpen, project, fetchWsConfig, fetchAgents]) + }, [isOpen, project, fetchWsConfig, fetchAgents, fetchPermsConfig]) useEffect(() => { if (wsConfig?.setup && wsConfig.setup.length > 0) { @@ -567,6 +579,30 @@ export function ProjectSettingsModal({ isOpen, onClose, project }: ProjectSettin )} +
+ +

+ Deterministic rules (not LLM-managed) that allow, deny, or force-ask for tool actions in this project. DENY + always wins. ALLOW skips sandbox and sensitive-file checks. ASK always prompts. Stored in{' '} + .openfox/permissions.json. +

+ { + await addRule('project', rule, project.workdir) + }} + onUpdate={async (index, rule) => { + await updateRule('project', index, rule, project.workdir) + }} + onDelete={async (index) => { + await deleteRule('project', index, project.workdir) + }} + /> +
+ {saveError && (
{saveError} diff --git a/web/src/components/settings/permissions-shared.tsx b/web/src/components/settings/permissions-shared.tsx new file mode 100644 index 00000000..b43379a6 --- /dev/null +++ b/web/src/components/settings/permissions-shared.tsx @@ -0,0 +1,303 @@ +import { useState, useEffect } from 'react' +import type { PermissionEffect, PermissionRule, PermissionScope, ScopedPermissionRule } from '@shared/permissions.js' +import { authFetch } from '../../lib/api' +import { Button } from '../shared/Button' +import { ConfirmModal } from '../shared/ConfirmModal' +import { EditSmallIcon, TrashIcon } from '../shared/icons' + +export const EFFECT_COLORS: Record = { + DENY: 'text-red-400 bg-red-500/10 border-red-500/30', + ALLOW: 'text-green-400 bg-green-500/10 border-green-500/30', + ASK: 'text-amber-400 bg-amber-500/10 border-amber-500/30', +} + +export const SCOPE_COLORS: Record = { + global: 'text-purple-400 bg-purple-500/10 border-purple-500/30', + project: 'text-blue-400 bg-blue-500/10 border-blue-500/30', +} + +export const EFFECTS: PermissionEffect[] = ['DENY', 'ALLOW', 'ASK'] + +const FALLBACK_TOOLS = ['read_file', 'write_file', 'edit_file', 'run_command'] + +const PATTERN_TOOLS = new Set(['read_file', 'write_file', 'edit_file', 'run_command']) + +export function EffectBadge({ effect }: { effect: PermissionEffect }) { + return {effect} +} + +export function ScopeBadge({ scope }: { scope: PermissionScope }) { + return {scope} +} + +export function RuleForm({ + initial, + initialScope = 'project', + onSave, + onCancel, + saving = false, + hideScope = false, + allowProject = true, +}: { + initial?: PermissionRule + initialScope?: PermissionScope + onSave: (rule: PermissionRule, scope: PermissionScope) => void + onCancel: () => void + saving?: boolean + hideScope?: boolean + allowProject?: boolean +}) { + const [effect, setEffect] = useState(initial?.effect ?? 'DENY') + const [tool, setTool] = useState(initial?.tool ?? 'read_file') + const [pattern, setPattern] = useState(initial?.pattern ?? '') + const [description, setDescription] = useState(initial?.description ?? '') + const [scope, setScope] = useState(!allowProject ? 'global' : (initialScope ?? 'project')) + const [tools, setTools] = useState(FALLBACK_TOOLS) + + useEffect(() => { + authFetch('/api/tools') + .then((r) => r.json()) + .then((d: { tools?: { name: string }[] }) => { + const names = (d.tools ?? []).map((t) => t.name) + if (names.length > 0) setTools(names) + }) + .catch(() => { + // fallback already set + }) + }, []) + + const handleSave = () => { + const rule: PermissionRule = { + effect, + tool, + ...(pattern.trim() ? { pattern: pattern.trim() } : {}), + ...(description.trim() ? { description: description.trim() } : {}), + } + onSave(rule, scope) + } + + const isCommandTool = tool === 'run_command' + const isPatternTool = PATTERN_TOOLS.has(tool) + const allowedEffects: PermissionEffect[] = isPatternTool ? EFFECTS : ['DENY'] + + return ( +
+
+
+ + +
+
+ + +
+ {!hideScope && ( +
+ + +
+ )} +
+
+ + setPattern(e.target.value)} + disabled={!isPatternTool} + placeholder={isCommandTool ? 'terragrunt destroy *' : isPatternTool ? '/path/** or **/.env*' : 'N/A'} + className="w-full px-2 py-1 text-sm font-mono text-text-primary bg-bg-primary border border-border rounded disabled:opacity-50 disabled:cursor-not-allowed" + /> +
+
+ + setDescription(e.target.value)} + placeholder="Why this rule exists" + className="w-full px-2 py-1 text-sm text-text-primary bg-bg-primary border border-border rounded" + /> +
+
+ + +
+
+ ) +} + +export function RuleRow({ + rule, + onEdit, + onDelete, +}: { + rule: ScopedPermissionRule + onEdit: () => void + onDelete: () => void +}) { + return ( +
+ + + {rule.tool} + {rule.pattern && {rule.pattern}} + {!rule.pattern && (all calls)} + {rule.description && ( + + — {rule.description} + + )} +
+ + +
+
+ ) +} + +export function PermissionsList({ + rules, + saving, + onAdd, + onUpdate, + onDelete, + emptyMessage = 'No permission rules. Add one to allow/deny tool actions without prompts.', + hideScope = false, + allowProject = true, +}: { + rules: ScopedPermissionRule[] + saving: boolean + onAdd: (rule: PermissionRule, scope: PermissionScope) => Promise + onUpdate: (index: number, rule: PermissionRule, scope: PermissionScope) => Promise + onDelete: (index: number, scope: PermissionScope) => Promise + emptyMessage?: string + hideScope?: boolean + allowProject?: boolean +}) { + const [showForm, setShowForm] = useState(false) + const [editIndex, setEditIndex] = useState(null) + const [deleteIndex, setDeleteIndex] = useState(null) + + const handleSave = async (rule: PermissionRule, scope: PermissionScope) => { + if (editIndex !== null) { + await onUpdate(editIndex, rule, scope) + } else { + await onAdd(rule, scope) + } + setShowForm(false) + setEditIndex(null) + } + + const handleDelete = async () => { + if (deleteIndex !== null) { + const rule = rules[deleteIndex] + if (rule) await onDelete(deleteIndex, rule.scope) + setDeleteIndex(null) + } + } + + return ( + <> + {showForm ? ( + { + setShowForm(false) + setEditIndex(null) + }} + saving={saving} + hideScope={hideScope || editIndex !== null} + allowProject={allowProject} + /> + ) : ( + + )} + + {rules.length === 0 && !showForm && ( +
{emptyMessage}
+ )} + + {rules.length > 0 && ( +
+ {rules.map((rule, i) => ( + { + setEditIndex(i) + setShowForm(true) + }} + onDelete={() => setDeleteIndex(i)} + /> + ))} +
+ )} + + setDeleteIndex(null)} + onConfirm={handleDelete} + title="Delete rule?" + message="This permission rule will be permanently removed." + confirmLabel="Delete" + confirmVariant="danger" + /> + + ) +} diff --git a/web/src/components/settings/tabs/PermissionsTab.test.tsx b/web/src/components/settings/tabs/PermissionsTab.test.tsx new file mode 100644 index 00000000..cfc258cd --- /dev/null +++ b/web/src/components/settings/tabs/PermissionsTab.test.tsx @@ -0,0 +1,299 @@ +/** + * @vitest-environment jsdom + */ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { cleanup, render, screen, waitFor } from '@testing-library/react' +import userEvent from '@testing-library/user-event' +import { PermissionsTab } from './PermissionsTab' +import { usePermissionsStore } from '../../../stores/permissions' +import type { PermissionConfig } from '@shared/permissions.js' + +vi.mock('../../../lib/api', () => ({ + authFetch: vi.fn(), +})) + +vi.mock('../../../lib/ws', () => ({ + wsClient: { send: vi.fn() }, +})) + +vi.mock('../../../stores/session', () => ({ + useSessionStore: vi.fn((selector) => { + const state = { currentSession: { workdir: '/test-workdir' } } + return selector ? selector(state) : state + }), +})) + +const mockAuthFetch = vi.mocked(await import('../../../lib/api').then((m) => m.authFetch)) + +function createJsonResponse(data: unknown, status = 200): Response { + return new Response(JSON.stringify(data), { + status, + headers: { 'content-type': 'application/json' }, + }) +} + +function mockConfigByScope(global?: Partial, project?: Partial) { + mockAuthFetch.mockImplementation(async (url: string) => { + if (url.includes('scope=global')) { + return createJsonResponse({ config: { version: 1, rules: [], ...global } }) + } + if (url.includes('scope=project')) { + return createJsonResponse({ config: { version: 1, rules: [], ...project } }) + } + if (url.includes('/api/tools')) { + return createJsonResponse({ tools: [] }) + } + return createJsonResponse({ config: { version: 1, rules: [] } }) + }) +} + +beforeEach(() => { + mockAuthFetch.mockReset() + usePermissionsStore.setState({ + globalConfig: null, + projectConfig: null, + mergedRules: [], + loading: false, + saving: false, + error: null, + }) +}) + +afterEach(() => { + cleanup() + vi.restoreAllMocks() +}) + +describe('PermissionsTab', () => { + it('shows empty state when no rules', async () => { + mockAuthFetch.mockResolvedValue(createJsonResponse({ config: { version: 1, rules: [] } })) + render() + await waitFor(() => { + expect(screen.getByText(/No permission rules/i)).toBeDefined() + }) + }) + + it('renders merged rules with scope badges', async () => { + const globalConfig: PermissionConfig = { + version: 1, + rules: [{ effect: 'DENY', tool: 'run_command', pattern: 'rm -rf *' }], + } + const projectConfig: PermissionConfig = { + version: 1, + rules: [{ effect: 'ALLOW', tool: 'read_file', pattern: '/ubiquity/**' }], + } + mockConfigByScope(globalConfig, projectConfig) + render() + await waitFor(() => { + expect(screen.getByText('rm -rf *')).toBeDefined() + expect(screen.getByText('/ubiquity/**')).toBeDefined() + expect(screen.getByText('global')).toBeDefined() + expect(screen.getByText('project')).toBeDefined() + }) + }) + + it('effect badges show correct colors (DENY=red, ALLOW=green, ASK=amber)', async () => { + const globalConfig: PermissionConfig = { + version: 1, + rules: [{ effect: 'DENY', tool: 'run_command', pattern: 'rm -rf *' }], + } + mockConfigByScope(globalConfig) + render() + await waitFor(() => { + const denyBadge = screen.getByText('DENY') + expect(denyBadge.className).toContain('red') + }) + }) + + it('add rule opens form with scope field', async () => { + mockAuthFetch.mockResolvedValue(createJsonResponse({ config: { version: 1, rules: [] } })) + render() + await waitFor(() => { + expect(screen.getByText(/No permission rules/i)).toBeDefined() + }) + + const user = userEvent.setup() + await user.click(screen.getByText(/Add Rule/i)) + await waitFor(() => { + expect(screen.getByText('Effect')).toBeDefined() + expect(screen.getByText('Tool')).toBeDefined() + expect(screen.getByText('Scope')).toBeDefined() + }) + }) + + it('delete rule shows confirm modal then removes', async () => { + const globalConfig: PermissionConfig = { + version: 1, + rules: [{ effect: 'DENY', tool: 'run_command', pattern: 'rm -rf *' }], + } + mockConfigByScope(globalConfig) + render() + await waitFor(() => { + expect(screen.getByText('rm -rf *')).toBeDefined() + }) + + mockAuthFetch.mockResolvedValueOnce(createJsonResponse({ config: { version: 1, rules: [] } })) + + const user = userEvent.setup() + const deleteButton = screen.getByTitle(/delete/i) + await user.click(deleteButton) + await waitFor(() => { + expect(screen.getByText('Delete rule?')).toBeDefined() + }) + const confirmBtn = screen.getByText('Delete', { selector: 'button' }) + await user.click(confirmBtn) + await waitFor(() => { + expect(screen.getByText(/No permission rules/i)).toBeDefined() + }) + }) + + it('renders without active session and shows note', async () => { + const mockSessionStore = vi.mocked(await import('../../../stores/session')).useSessionStore + const impl = (selector: unknown) => { + const state = { currentSession: null } + return typeof selector === 'function' ? (selector as (s: typeof state) => unknown)(state) : state + } + mockSessionStore.mockImplementation(impl as never) + mockConfigByScope() + render() + await waitFor(() => { + expect(screen.getByText(/No active project/i)).toBeDefined() + }) + }) + + it('disables Project scope in add form when no active session', async () => { + const mockSessionStore = vi.mocked(await import('../../../stores/session')).useSessionStore + const impl = (selector: unknown) => { + const state = { currentSession: null } + return typeof selector === 'function' ? (selector as (s: typeof state) => unknown)(state) : state + } + mockSessionStore.mockImplementation(impl as never) + mockConfigByScope() + render() + await waitFor(() => { + expect(screen.getByText(/No active project/i)).toBeDefined() + }) + const user = userEvent.setup() + await user.click(screen.getByText(/Add Rule/i)) + await waitFor(() => { + const projectOption = screen.getByText(/Project \(no active session\)/) as HTMLOptionElement + expect(projectOption.disabled).toBe(true) + }) + }) + + it('renders error state with retry button', async () => { + mockAuthFetch.mockRejectedValue(new Error('HTTP 500')) + render() + await waitFor(() => { + expect(screen.getByText(/HTTP 500/)).toBeDefined() + expect(screen.getByText(/Retry/i)).toBeDefined() + }) + }) + + it('displays description in RuleRow when present', async () => { + const globalConfig: PermissionConfig = { + version: 1, + rules: [{ effect: 'DENY', tool: 'run_command', pattern: 'rm -rf *', description: 'Never delete recursively' }], + } + mockConfigByScope(globalConfig) + render() + await waitFor(() => { + expect(screen.getByText(/Never delete recursively/)).toBeDefined() + }) + }) + + it('does not render description text when absent', async () => { + const globalConfig: PermissionConfig = { + version: 1, + rules: [{ effect: 'DENY', tool: 'run_command', pattern: 'rm -rf *' }], + } + mockConfigByScope(globalConfig) + render() + await waitFor(() => { + expect(screen.getByText('rm -rf *')).toBeDefined() + }) + expect(screen.queryByText(/Never delete recursively/)).toBeNull() + }) + + it('shows command-specific pattern hint for run_command', async () => { + mockAuthFetch.mockResolvedValue(createJsonResponse({ config: { version: 1, rules: [] } })) + render() + await waitFor(() => { + expect(screen.getByText(/No permission rules/i)).toBeDefined() + }) + const user = userEvent.setup() + await user.click(screen.getByText(/Add Rule/i)) + await waitFor(() => { + expect(screen.getByText('Effect')).toBeDefined() + }) + const toolSelect = screen.getByDisplayValue('read_file') + await user.selectOptions(toolSelect, 'run_command') + await waitFor(() => { + expect(screen.getByText(/matches anything/i)).toBeDefined() + }) + }) + + it('shows path-specific pattern hint for read_file', async () => { + mockAuthFetch.mockResolvedValue(createJsonResponse({ config: { version: 1, rules: [] } })) + render() + await waitFor(() => { + expect(screen.getByText(/No permission rules/i)).toBeDefined() + }) + const user = userEvent.setup() + await user.click(screen.getByText(/Add Rule/i)) + const toolSelect = screen.getByDisplayValue('read_file') + await user.click(toolSelect) + await waitFor(() => { + expect(screen.getByText(/any depth/i)).toBeDefined() + }) + }) + + it('restricts effect to DENY-only for non-pattern tools (web_fetch)', async () => { + mockAuthFetch.mockImplementation(async (url: string) => { + if (url.includes('/api/tools')) { + return createJsonResponse({ tools: [{ name: 'read_file' }, { name: 'run_command' }, { name: 'web_fetch' }] }) + } + return createJsonResponse({ config: { version: 1, rules: [] } }) + }) + render() + await waitFor(() => { + expect(screen.getByText(/No permission rules/i)).toBeDefined() + }) + const user = userEvent.setup() + await user.click(screen.getByText(/Add Rule/i)) + await waitFor(() => { + expect(screen.getByText('Effect')).toBeDefined() + }) + const toolSelect = screen.getByDisplayValue('read_file') + await user.selectOptions(toolSelect, 'web_fetch') + await waitFor(() => { + const effectSelect = screen.getByDisplayValue('DENY') as HTMLSelectElement + const options = Array.from(effectSelect.options).map((o) => o.value) + expect(options).toEqual(['DENY']) + }) + }) + + it('disables pattern input for non-pattern tools (web_fetch)', async () => { + mockAuthFetch.mockImplementation(async (url: string) => { + if (url.includes('/api/tools')) { + return createJsonResponse({ tools: [{ name: 'read_file' }, { name: 'run_command' }, { name: 'web_fetch' }] }) + } + return createJsonResponse({ config: { version: 1, rules: [] } }) + }) + render() + await waitFor(() => { + expect(screen.getByText(/No permission rules/i)).toBeDefined() + }) + const user = userEvent.setup() + await user.click(screen.getByText(/Add Rule/i)) + await waitFor(() => { + expect(screen.getByText('Effect')).toBeDefined() + }) + const toolSelect = screen.getByDisplayValue('read_file') + await user.selectOptions(toolSelect, 'web_fetch') + await waitFor(() => { + const patternInput = screen.getByPlaceholderText('N/A') as HTMLInputElement + expect(patternInput.disabled).toBe(true) + }) + }) +}) diff --git a/web/src/components/settings/tabs/PermissionsTab.tsx b/web/src/components/settings/tabs/PermissionsTab.tsx new file mode 100644 index 00000000..1210d0c5 --- /dev/null +++ b/web/src/components/settings/tabs/PermissionsTab.tsx @@ -0,0 +1,73 @@ +import { useEffect } from 'react' +import { usePermissionsStore } from '../../../stores/permissions' +import { useSessionStore } from '../../../stores/session' +import { PermissionsList } from '../permissions-shared' +import type { PermissionScope } from '@shared/permissions.js' + +export function PermissionsTab() { + const { mergedRules, loading, saving, error, fetchAll, addRule, updateRule, deleteRule } = usePermissionsStore() + const currentSession = useSessionStore((s) => s.currentSession) + const workdir = currentSession?.workspace ?? currentSession?.workdir + + useEffect(() => { + fetchAll(workdir) + }, [fetchAll, workdir]) + + if (loading && mergedRules.length === 0) { + return
Loading permissions...
+ } + + // Map a merged-list display index to the index within its scope's config. + // mergedRules = [...globalRules, ...projectRules], each tagged with scope. + const scopeIndexOf = (displayIndex: number): { scope: PermissionScope; index: number } => { + const rule = mergedRules[displayIndex] + if (!rule) return { scope: 'project' as const, index: 0 } + let index = 0 + for (let i = 0; i < displayIndex; i++) { + if (mergedRules[i]!.scope === rule.scope) index++ + } + return { scope: rule.scope, index } + } + + return ( +
+ {error && ( +
+ Failed to load permissions: {error} + +
+ )} + {!workdir && ( +
+ No active project — showing global rules only. +
+ )} +
+

Permission Rules

+

+ Deterministic rules (not LLM-managed) that allow, deny, or force-ask for tool actions. DENY always wins, even + in dangerous mode. ALLOW skips sandbox and sensitive-file checks for matching paths. Project rules are stored + in .openfox/permissions.json, global rules in your OpenFox config directory. +

+
+ { + await addRule(scope, rule, workdir) + }} + onUpdate={async (displayIndex, rule) => { + const { scope, index } = scopeIndexOf(displayIndex) + await updateRule(scope, index, rule, workdir) + }} + onDelete={async (displayIndex) => { + const { scope, index } = scopeIndexOf(displayIndex) + await deleteRule(scope, index, workdir) + }} + /> +
+ ) +} diff --git a/web/src/components/shared/PathConfirmationButtons.test.tsx b/web/src/components/shared/PathConfirmationButtons.test.tsx new file mode 100644 index 00000000..1ee58599 --- /dev/null +++ b/web/src/components/shared/PathConfirmationButtons.test.tsx @@ -0,0 +1,168 @@ +// @vitest-environment happy-dom +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { createRoot } from 'react-dom/client' +import { act } from 'react' + +const { confirmPathMock, switchDangerLevelMock } = vi.hoisted(() => ({ + confirmPathMock: vi.fn(), + switchDangerLevelMock: vi.fn(), +})) + +vi.mock('../../lib/ws', () => ({ + wsClient: { + connect: vi.fn(), + disconnect: vi.fn(), + send: vi.fn(), + subscribe: vi.fn(), + onStatusChange: vi.fn(), + }, +})) + +interface MockStore { + (selector?: (state: any) => any): any + setState: (partial: Record) => void +} + +function mockStore(initial: Record): MockStore { + let state = { ...initial } + const fn = vi.fn((selector?: (s: typeof state) => any) => { + return selector ? selector(state) : state + }) as unknown as MockStore + fn.setState = (partial: Record) => { + state = { ...state, ...partial } + } + return fn +} + +vi.mock('../../stores/session', () => ({ + useSessionStore: mockStore({ + confirmPath: confirmPathMock, + switchDangerLevel: switchDangerLevelMock, + }), +})) + +vi.mock('../../stores/session/session-scope', () => ({ + useSessionScope: () => 'session-1', +})) + +vi.mock('./ScrollArea', () => ({ + ScrollArea: ({ children }: any) =>
{children}
, +})) + +vi.mock('./icons', () => ({ + WarningSmallIcon: () => , +})) + +import { PathConfirmationButtons } from './PathConfirmationButtons' +import type { PendingPathConfirmation } from '../../stores/session' + +function renderConfirmation(reason: PendingPathConfirmation['reason']): HTMLElement { + const container = document.createElement('div') + document.body.appendChild(container) + const root = createRoot(container) + const confirmation: PendingPathConfirmation = { + callId: 'call-1', + tool: 'read_file', + paths: ['/some/path'], + workdir: '/workdir', + reason, + } + act(() => { + root.render() + }) + return container +} + +function getButtons(container: HTMLElement): HTMLButtonElement[] { + return Array.from(container.querySelectorAll('button')) +} + +function getButtonByText(container: HTMLElement, text: string): HTMLButtonElement | undefined { + return getButtons(container).find((b) => b.textContent?.includes(text)) +} + +beforeEach(() => { + confirmPathMock.mockClear() + switchDangerLevelMock.mockClear() +}) + +describe('PathConfirmationButtons', () => { + it('renders "Allow for this session" button for rule_ask', () => { + const container = renderConfirmation('rule_ask') + expect(getButtonByText(container, 'Allow for this session')).toBeDefined() + }) + + it('renders "Allow for this session" button for outside_workdir', () => { + const container = renderConfirmation('outside_workdir') + expect(getButtonByText(container, 'Allow for this session')).toBeDefined() + }) + + it('renders "Allow for this session" button for sensitive_file', () => { + const container = renderConfirmation('sensitive_file') + expect(getButtonByText(container, 'Allow for this session')).toBeDefined() + }) + + it('hides "Allow for this session" button for dangerous_command (no persistent allow)', () => { + const container = renderConfirmation('dangerous_command') + const btn = getButtonByText(container, 'Allow for this session') + expect(btn?.className).toContain('hidden') + }) + + it('hides "Allow for this session" button for git_no_verify (no persistent allow)', () => { + const container = renderConfirmation('git_no_verify') + const btn = getButtonByText(container, 'Allow for this session') + expect(btn?.className).toContain('hidden') + }) + + it('clicking "Allow for this session" calls confirmPath with alwaysAllow=true', () => { + const container = renderConfirmation('rule_ask') + const btn = getButtonByText(container, 'Allow for this session')! + act(() => { + btn.dispatchEvent(new MouseEvent('click', { bubbles: true })) + }) + expect(confirmPathMock).toHaveBeenCalledWith('session-1', 'call-1', true, true) + }) + + it('clicking "Allow for this session" does NOT switch dangerous mode', () => { + const container = renderConfirmation('rule_ask') + const btn = getButtonByText(container, 'Allow for this session')! + act(() => { + btn.dispatchEvent(new MouseEvent('click', { bubbles: true })) + }) + expect(switchDangerLevelMock).not.toHaveBeenCalled() + }) + + it('clicking "Allow" calls confirmPath with alwaysAllow=false', () => { + const container = renderConfirmation('rule_ask') + const btn = getButtonByText(container, 'Allow')! + act(() => { + btn.dispatchEvent(new MouseEvent('click', { bubbles: true })) + }) + expect(confirmPathMock).toHaveBeenCalledWith('session-1', 'call-1', true, false) + }) + + it('clicking "Deny" calls confirmPath with approved=false', () => { + const container = renderConfirmation('rule_ask') + const btn = getButtonByText(container, 'Deny')! + act(() => { + btn.dispatchEvent(new MouseEvent('click', { bubbles: true })) + }) + expect(confirmPathMock).toHaveBeenCalledWith('session-1', 'call-1', false) + }) + + it('"Allow Everything" button is present and switches dangerous mode', () => { + const container = renderConfirmation('rule_ask') + const btn = getButtonByText(container, 'Allow Everything')! + act(() => { + btn.dispatchEvent(new MouseEvent('click', { bubbles: true })) + }) + expect(switchDangerLevelMock).toHaveBeenCalledWith('session-1', 'dangerous') + expect(confirmPathMock).toHaveBeenCalledWith('session-1', 'call-1', true, false) + }) + + it('"Allow Everything" is hidden for git_no_verify', () => { + const container = renderConfirmation('git_no_verify') + const btn = getButtonByText(container, 'Allow Everything') + expect(btn?.className).toContain('hidden') + }) +}) diff --git a/web/src/components/shared/PathConfirmationButtons.tsx b/web/src/components/shared/PathConfirmationButtons.tsx index edfd4615..b6748866 100644 --- a/web/src/components/shared/PathConfirmationButtons.tsx +++ b/web/src/components/shared/PathConfirmationButtons.tsx @@ -32,6 +32,11 @@ function getReasonMessage(reason: PendingPathConfirmation['reason']): { title: 'Git --no-verify', description: 'Bypassing git hooks/pre-commit checks', } + case 'rule_ask': + return { + title: 'Permission Rule Confirmation', + description: 'A permission rule requires confirmation for this action', + } case 'outside_workdir': default: return { @@ -52,6 +57,8 @@ export function PathConfirmationButtons({ confirmation }: PathConfirmationButton const bgColor = isSensitive ? 'bg-red-500/10' : 'bg-amber-500/10' const isGitNoVerify = confirmation.reason === 'git_no_verify' + const isDangerousCommand = confirmation.reason === 'dangerous_command' + const canAllowForSession = !isGitNoVerify && !isDangerousCommand const handleEnableDangerousAndAllow = () => { if (!sessionId) return @@ -59,6 +66,11 @@ export function PathConfirmationButtons({ confirmation }: PathConfirmationButton confirmPath(sessionId, confirmation.callId, true, false) } + const handleAllowForSession = () => { + if (!sessionId) return + confirmPath(sessionId, confirmation.callId, true, true) + } + return (
@@ -96,6 +108,13 @@ export function PathConfirmationButtons({ confirmation }: PathConfirmationButton > Allow +