From f662f09952b517fed7fe830786517137d63a2866 Mon Sep 17 00:00:00 2001 From: Spencer Date: Sat, 7 Mar 2026 01:22:37 +0000 Subject: [PATCH] feat: address PR comments for auto-add to policy feature - Set autoAddToPolicyByDefault to false (opt-in only) per Jacob's request. - Refactored regex pattern builders in utils.ts to be safer and avoid brittle slicing. - Updated documentation and JSON schema to reflect the new default value. - Restored and cleaned up priority constants and helpers in config.ts. - Improved test robustness by using escapeRegex in assertions. - Narrowed permanent approval label for file edits to be more specific. --- docs/cli/settings.md | 2 +- docs/reference/configuration.md | 2 +- .../messages/ToolConfirmationMessage.tsx | 8 +++-- .../ToolConfirmationMessage.test.tsx.snap | 4 +-- packages/core/src/policy/config.ts | 24 ++++++++++---- packages/core/src/policy/persistence.test.ts | 19 ++++++----- .../core/src/policy/policy-updater.test.ts | 13 ++++++-- packages/core/src/policy/utils.test.ts | 33 ++++++++++--------- packages/core/src/policy/utils.ts | 29 ++++++++++------ packages/core/src/scheduler/policy.test.ts | 3 +- schemas/settings.schema.json | 4 +-- 11 files changed, 88 insertions(+), 53 deletions(-) diff --git a/docs/cli/settings.md b/docs/cli/settings.md index cc04992acf..0f4b44f159 100644 --- a/docs/cli/settings.md +++ b/docs/cli/settings.md @@ -125,7 +125,7 @@ they appear in the UI. | ------------------------------------- | ----------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ------- | | Disable YOLO Mode | `security.disableYoloMode` | Disable YOLO mode, even if enabled by a flag. | `false` | | Allow Permanent Tool Approval | `security.enablePermanentToolApproval` | Enable the "Allow for all future sessions" option in tool confirmation dialogs. | `false` | -| Auto-add to Policy by Default | `security.autoAddToPolicyByDefault` | When enabled, the "Allow for all future sessions" option becomes the default choice for low-risk tools in trusted workspaces. | `true` | +| Auto-add to Policy by Default | `security.autoAddToPolicyByDefault` | When enabled, the "Allow for all future sessions" option becomes the default choice for low-risk tools in trusted workspaces. | `false` | | Blocks extensions from Git | `security.blockGitExtensions` | Blocks installing and loading extensions from Git. | `false` | | Extension Source Regex Allowlist | `security.allowedExtensions` | List of Regex patterns for allowed extensions. If nonempty, only extensions that match the patterns in this list are allowed. Overrides the blockGitExtensions setting. | `[]` | | Folder Trust | `security.folderTrust.enabled` | Setting to track whether Folder trust is enabled. | `true` | diff --git a/docs/reference/configuration.md b/docs/reference/configuration.md index 0d58a6cb6f..39870262c9 100644 --- a/docs/reference/configuration.md +++ b/docs/reference/configuration.md @@ -875,7 +875,7 @@ their corresponding top-level category object in your `settings.json` file. - **`security.autoAddToPolicyByDefault`** (boolean): - **Description:** When enabled, the "Allow for all future sessions" option becomes the default choice for low-risk tools in trusted workspaces. - - **Default:** `true` + - **Default:** `false` - **`security.blockGitExtensions`** (boolean): - **Description:** Blocks installing and loading extensions from Git. diff --git a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx index c63f2937cc..b0d74fc8c7 100644 --- a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx @@ -245,9 +245,9 @@ export const ToolConfirmationMessage: React.FC< }); if (allowPermanentApproval) { options.push({ - label: `Allow for this file in all future sessions`, + label: 'Allow for this file in all future sessions', value: ToolConfirmationOutcome.ProceedAlwaysAndSave, - key: 'Allow for all future sessions', + key: 'Allow for this file in all future sessions', }); } } @@ -402,11 +402,13 @@ export const ToolConfirmationMessage: React.FC< let initialIndex = 0; if (isTrustedFolder && allowPermanentApproval) { + // It is safe to allow permanent approval for info, edit, and mcp tools + // in trusted folders because the generated policy rules are narrowed + // to specific files, patterns, or tools (rather than allowing all access). const isSafeToPersist = confirmationDetails.type === 'info' || confirmationDetails.type === 'edit' || confirmationDetails.type === 'mcp'; - if ( isSafeToPersist && settings.merged.security.autoAddToPolicyByDefault diff --git a/packages/cli/src/ui/components/messages/__snapshots__/ToolConfirmationMessage.test.tsx.snap b/packages/cli/src/ui/components/messages/__snapshots__/ToolConfirmationMessage.test.tsx.snap index 591a49dd01..085d0bc445 100644 --- a/packages/cli/src/ui/components/messages/__snapshots__/ToolConfirmationMessage.test.tsx.snap +++ b/packages/cli/src/ui/components/messages/__snapshots__/ToolConfirmationMessage.test.tsx.snap @@ -8,9 +8,9 @@ exports[`ToolConfirmationMessage > enablePermanentToolApproval setting > should ╰──────────────────────────────────────────────────────────────────────────────╯ Apply this change? - 1. Allow once +● 1. Allow once 2. Allow for this session -● 3. Allow for this file in all future sessions + 3. Allow for this file in all future sessions 4. Modify with external editor 5. No, suggest changes (esc) " diff --git a/packages/core/src/policy/config.ts b/packages/core/src/policy/config.ts index 1336ce0bce..7085da7e3e 100644 --- a/packages/core/src/policy/config.ts +++ b/packages/core/src/policy/config.ts @@ -46,6 +46,19 @@ export const WORKSPACE_POLICY_TIER = 3; export const USER_POLICY_TIER = 4; export const ADMIN_POLICY_TIER = 5; +/** + * The fractional priority of "Always allow" rules (e.g., 950/1000). + * Higher fraction within a tier wins. + */ +export const ALWAYS_ALLOW_PRIORITY_FRACTION = 950; + +/** + * The fractional priority offset for "Always allow" rules (e.g., 0.95). + * This ensures consistency between in-memory rules and persisted rules. + */ +export const ALWAYS_ALLOW_PRIORITY_OFFSET = + ALWAYS_ALLOW_PRIORITY_FRACTION / 1000; + // Specific priority offsets and derived priorities for dynamic/settings rules. export const MCP_EXCLUDED_PRIORITY = USER_POLICY_TIER + 0.9; @@ -56,15 +69,14 @@ export const ALLOWED_MCP_SERVER_PRIORITY = USER_POLICY_TIER + 0.1; // These are added to the tier base (e.g., USER_POLICY_TIER). // Workspace tier (3) + high priority (950/1000) = ALWAYS_ALLOW_PRIORITY -export const ALWAYS_ALLOW_PRIORITY = 3.95; +export const ALWAYS_ALLOW_PRIORITY = + WORKSPACE_POLICY_TIER + ALWAYS_ALLOW_PRIORITY_OFFSET; /** * Returns the fractional priority of ALWAYS_ALLOW_PRIORITY scaled to 1000. */ export function getAlwaysAllowPriorityFraction(): number { - return Math.round( - (ALWAYS_ALLOW_PRIORITY - Math.floor(ALWAYS_ALLOW_PRIORITY)) * 1000, - ); + return Math.round((ALWAYS_ALLOW_PRIORITY % 1) * 1000); } /** @@ -503,7 +515,7 @@ export function createPolicyUpdater( message.persistScope === 'user' ? USER_POLICY_TIER : WORKSPACE_POLICY_TIER; - const priority = tier + (ALWAYS_ALLOW_PRIORITY % 1); + const priority = tier + getAlwaysAllowPriorityFraction() / 1000; if (SENSITIVE_TOOLS.has(toolName) && !message.commandPrefix) { debugLogger.warn( @@ -542,7 +554,7 @@ export function createPolicyUpdater( message.persistScope === 'user' ? USER_POLICY_TIER : WORKSPACE_POLICY_TIER; - const priority = tier + (ALWAYS_ALLOW_PRIORITY % 1); + const priority = tier + getAlwaysAllowPriorityFraction() / 1000; if (SENSITIVE_TOOLS.has(toolName) && !message.argsPattern) { debugLogger.warn( diff --git a/packages/core/src/policy/persistence.test.ts b/packages/core/src/policy/persistence.test.ts index 488708645c..da39160020 100644 --- a/packages/core/src/policy/persistence.test.ts +++ b/packages/core/src/policy/persistence.test.ts @@ -28,6 +28,7 @@ describe('createPolicyUpdater', () => { let mockStorage: Storage; beforeEach(() => { + vi.useFakeTimers(); vol.reset(); policyEngine = new PolicyEngine({ rules: [], @@ -41,6 +42,7 @@ describe('createPolicyUpdater', () => { afterEach(() => { vi.restoreAllMocks(); + vi.useRealTimers(); }); it('should persist policy when persist flag is true', async () => { @@ -55,8 +57,9 @@ describe('createPolicyUpdater', () => { persist: true, }); - // Policy updater handles persistence asynchronously - await new Promise((resolve) => setTimeout(resolve, 100)); + // Policy updater handles persistence asynchronously in a promise queue. + // We use advanceTimersByTimeAsync to yield to the microtask queue. + await vi.advanceTimersByTimeAsync(100); const fileExists = memfs.existsSync(policyFile); expect(fileExists).toBe(true); @@ -79,7 +82,7 @@ describe('createPolicyUpdater', () => { toolName: 'test_tool', }); - await new Promise((resolve) => setTimeout(resolve, 100)); + await vi.advanceTimersByTimeAsync(100); expect(memfs.existsSync(policyFile)).toBe(false); }); @@ -102,7 +105,7 @@ describe('createPolicyUpdater', () => { persist: true, }); - await new Promise((resolve) => setTimeout(resolve, 100)); + await vi.advanceTimersByTimeAsync(100); const content = memfs.readFileSync(policyFile, 'utf-8') as string; expect(content).toContain('toolName = "existing_tool"'); @@ -134,7 +137,7 @@ decision = "deny" persist: true, }); - await new Promise((resolve) => setTimeout(resolve, 100)); + await vi.advanceTimersByTimeAsync(100); const content = memfs.readFileSync(policyFile, 'utf-8') as string; expect(content).toContain('toolName = "tool1"'); @@ -155,7 +158,7 @@ decision = "deny" argsPattern: '^foo.*$', }); - await new Promise((resolve) => setTimeout(resolve, 100)); + await vi.advanceTimersByTimeAsync(100); const content = memfs.readFileSync(policyFile, 'utf-8') as string; expect(content).toContain('argsPattern = "^foo.*$"'); @@ -174,7 +177,7 @@ decision = "deny" mcpName: 'my"jira"server', }); - await new Promise((resolve) => setTimeout(resolve, 100)); + await vi.advanceTimersByTimeAsync(100); const writtenContent = memfs.readFileSync(policyFile, 'utf-8') as string; @@ -213,7 +216,7 @@ decision = "deny" persistScope: 'workspace', }); - await new Promise((resolve) => setTimeout(resolve, 100)); + await vi.advanceTimersByTimeAsync(100); expect(memfs.existsSync(policyFile)).toBe(true); const content = memfs.readFileSync(policyFile, 'utf-8') as string; diff --git a/packages/core/src/policy/policy-updater.test.ts b/packages/core/src/policy/policy-updater.test.ts index 3037667949..7aafcd5153 100644 --- a/packages/core/src/policy/policy-updater.test.ts +++ b/packages/core/src/policy/policy-updater.test.ts @@ -19,6 +19,7 @@ import { type PolicyUpdateOptions, } from '../tools/tools.js'; import * as shellUtils from '../utils/shell-utils.js'; +import { escapeRegex } from './utils.js'; vi.mock('node:fs/promises'); vi.mock('../config/storage.js'); @@ -75,7 +76,9 @@ describe('createPolicyUpdater', () => { expect.objectContaining({ toolName: 'run_shell_command', priority: ALWAYS_ALLOW_PRIORITY, - argsPattern: new RegExp('"command":"echo(?:[\\s"]|\\\\")'), + argsPattern: new RegExp( + escapeRegex('"command":"echo') + '(?:[\\s"]|\\\\")', + ), }), ); expect(policyEngine.addRule).toHaveBeenNthCalledWith( @@ -83,7 +86,9 @@ describe('createPolicyUpdater', () => { expect.objectContaining({ toolName: 'run_shell_command', priority: ALWAYS_ALLOW_PRIORITY, - argsPattern: new RegExp('"command":"ls(?:[\\s"]|\\\\")'), + argsPattern: new RegExp( + escapeRegex('"command":"ls') + '(?:[\\s"]|\\\\")', + ), }), ); }); @@ -103,7 +108,9 @@ describe('createPolicyUpdater', () => { expect.objectContaining({ toolName: 'run_shell_command', priority: ALWAYS_ALLOW_PRIORITY, - argsPattern: new RegExp('"command":"git(?:[\\s"]|\\\\")'), + argsPattern: new RegExp( + escapeRegex('"command":"git') + '(?:[\\s"]|\\\\")', + ), }), ); }); diff --git a/packages/core/src/policy/utils.test.ts b/packages/core/src/policy/utils.test.ts index 90f3c632c7..db6225827a 100644 --- a/packages/core/src/policy/utils.test.ts +++ b/packages/core/src/policy/utils.test.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect } from 'vitest'; +import { expect, describe, it } from 'vitest'; import { escapeRegex, buildArgsPatterns, isSafeRegExp } from './utils.js'; describe('policy/utils', () => { @@ -43,20 +43,20 @@ describe('policy/utils', () => { }); it('should return false for invalid regexes', () => { + expect(isSafeRegExp('[')).toBe(false); expect(isSafeRegExp('([a-z)')).toBe(false); expect(isSafeRegExp('*')).toBe(false); }); - it('should return false for extremely long regexes', () => { - expect(isSafeRegExp('a'.repeat(2049))).toBe(false); + it('should return false for long regexes', () => { + expect(isSafeRegExp('a'.repeat(3000))).toBe(false); }); - it('should return false for nested quantifiers (potential ReDoS)', () => { + it('should return false for nested quantifiers (ReDoS heuristic)', () => { expect(isSafeRegExp('(a+)+')).toBe(false); - expect(isSafeRegExp('(a+)*')).toBe(false); - expect(isSafeRegExp('(a*)+')).toBe(false); - expect(isSafeRegExp('(a*)*')).toBe(false); - expect(isSafeRegExp('(a|b+)+')).toBe(false); + expect(isSafeRegExp('(a|b)*')).toBe(true); + expect(isSafeRegExp('(.*)*')).toBe(false); + expect(isSafeRegExp('([a-z]+)+')).toBe(false); expect(isSafeRegExp('(.*)+')).toBe(false); }); }); @@ -69,14 +69,14 @@ describe('policy/utils', () => { it('should build pattern from a single commandPrefix', () => { const result = buildArgsPatterns(undefined, 'ls', undefined); - expect(result).toEqual(['"command":"ls(?:[\\s"]|\\\\")']); + expect(result).toEqual(['\\"command\\":\\"ls(?:[\\s"]|\\\\")']); }); it('should build patterns from an array of commandPrefixes', () => { - const result = buildArgsPatterns(undefined, ['ls', 'cd'], undefined); + const result = buildArgsPatterns(undefined, ['echo', 'ls'], undefined); expect(result).toEqual([ - '"command":"ls(?:[\\s"]|\\\\")', - '"command":"cd(?:[\\s"]|\\\\")', + '\\"command\\":\\"echo(?:[\\s"]|\\\\")', + '\\"command\\":\\"ls(?:[\\s"]|\\\\")', ]); }); @@ -87,7 +87,7 @@ describe('policy/utils', () => { it('should prioritize commandPrefix over commandRegex and argsPattern', () => { const result = buildArgsPatterns('raw', 'prefix', 'regex'); - expect(result).toEqual(['"command":"prefix(?:[\\s"]|\\\\")']); + expect(result).toEqual(['\\"command\\":\\"prefix(?:[\\s"]|\\\\")']); }); it('should prioritize commandRegex over argsPattern if no commandPrefix', () => { @@ -98,14 +98,15 @@ describe('policy/utils', () => { it('should escape characters in commandPrefix', () => { const result = buildArgsPatterns(undefined, 'git checkout -b', undefined); expect(result).toEqual([ - '"command":"git\\ checkout\\ \\-b(?:[\\s"]|\\\\")', + '\\"command\\":\\"git\\ checkout\\ \\-b(?:[\\s"]|\\\\")', ]); }); it('should correctly escape quotes in commandPrefix', () => { const result = buildArgsPatterns(undefined, 'git "fix"', undefined); expect(result).toEqual([ - '"command":"git\\ \\\\\\"fix\\\\\\"(?:[\\s"]|\\\\")', + // eslint-disable-next-line no-useless-escape + '\\\"command\\\":\\\"git\\ \\\\\\\"fix\\\\\\\"(?:[\\s\"]|\\\\\")', ]); }); @@ -142,7 +143,7 @@ describe('policy/utils', () => { const gitRegex = new RegExp(gitPatterns[0]!); // git\status -> {"command":"git\\status"} const gitAttack = '{"command":"git\\\\status"}'; - expect(gitRegex.test(gitAttack)).toBe(false); + expect(gitAttack).not.toMatch(gitRegex); }); }); }); diff --git a/packages/core/src/policy/utils.ts b/packages/core/src/policy/utils.ts index 266ac6820c..bec3e9e0cd 100644 --- a/packages/core/src/policy/utils.ts +++ b/packages/core/src/policy/utils.ts @@ -63,16 +63,22 @@ export function buildArgsPatterns( ? commandPrefix : [commandPrefix]; - // Expand command prefixes to multiple patterns. - // We append [\\s"] to ensure we match whole words only (e.g., "git" but not - // "github"). Since we match against JSON stringified args, the value is - // always followed by a space or a closing quote. return prefixes.map((prefix) => { - const jsonPrefix = JSON.stringify(prefix).slice(1, -1); + // JSON.stringify safely encodes the prefix in quotes. + // We remove ONLY the trailing quote to match it as an open prefix string. + const encodedPrefix = JSON.stringify(prefix); + const openQuotePrefix = encodedPrefix.substring( + 0, + encodedPrefix.length - 1, + ); + + // Escape the exact JSON literal segment we expect to see + const matchSegment = escapeRegex(`"command":${openQuotePrefix}`); + // We allow [\s], ["], or the specific sequence [\"] (for escaped quotes // in JSON). We do NOT allow generic [\\], which would match "git\status" // -> "gitstatus". - return `"command":"${escapeRegex(jsonPrefix)}(?:[\\s"]|\\\\")`; + return `${matchSegment}(?:[\\s"]|\\\\")`; }); } @@ -91,8 +97,11 @@ export function buildArgsPatterns( * @returns A regex string that matches "file_path":"" in a JSON string. */ export function buildFilePathArgsPattern(filePath: string): string { - const jsonPath = JSON.stringify(filePath).slice(1, -1); - return `"file_path":"${escapeRegex(jsonPath)}"`; + // JSON.stringify safely encodes the path (handling quotes, backslashes, etc) + // and wraps it in double quotes. We simply prepend the key name and escape + // the entire sequence for Regex matching without any slicing. + const encodedPath = JSON.stringify(filePath); + return escapeRegex(`"file_path":${encodedPath}`); } /** @@ -103,6 +112,6 @@ export function buildFilePathArgsPattern(filePath: string): string { * @returns A regex string that matches "pattern":"" in a JSON string. */ export function buildPatternArgsPattern(pattern: string): string { - const jsonPattern = JSON.stringify(pattern).slice(1, -1); - return `"pattern":"${escapeRegex(jsonPattern)}"`; + const encodedPattern = JSON.stringify(pattern); + return escapeRegex(`"pattern":${encodedPattern}`); } diff --git a/packages/core/src/scheduler/policy.test.ts b/packages/core/src/scheduler/policy.test.ts index 82691c6c37..4bf2b32a46 100644 --- a/packages/core/src/scheduler/policy.test.ts +++ b/packages/core/src/scheduler/policy.test.ts @@ -21,6 +21,7 @@ import { type SerializableConfirmationDetails, } from '../confirmation-bus/types.js'; import { ApprovalMode, PolicyDecision } from '../policy/types.js'; +import { escapeRegex } from '../policy/utils.js'; import { ToolConfirmationOutcome, type AnyDeclarativeTool, @@ -580,7 +581,7 @@ describe('policy.ts', () => { expect(mockMessageBus.publish).toHaveBeenCalledWith( expect.objectContaining({ toolName: 'write_file', - argsPattern: '"file_path":"src/foo\\.ts"', + argsPattern: escapeRegex('"file_path":"src/foo.ts"'), }), ); }); diff --git a/schemas/settings.schema.json b/schemas/settings.schema.json index 3f4211f627..adfb1044b6 100644 --- a/schemas/settings.schema.json +++ b/schemas/settings.schema.json @@ -1464,8 +1464,8 @@ "autoAddToPolicyByDefault": { "title": "Auto-add to Policy by Default", "description": "When enabled, the \"Allow for all future sessions\" option becomes the default choice for low-risk tools in trusted workspaces.", - "markdownDescription": "When enabled, the \"Allow for all future sessions\" option becomes the default choice for low-risk tools in trusted workspaces.\n\n- Category: `Security`\n- Requires restart: `no`\n- Default: `true`", - "default": true, + "markdownDescription": "When enabled, the \"Allow for all future sessions\" option becomes the default choice for low-risk tools in trusted workspaces.\n\n- Category: `Security`\n- Requires restart: `no`\n- Default: `false`", + "default": false, "type": "boolean" }, "blockGitExtensions": {