mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-05-12 21:03:05 -07:00
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.
This commit is contained in:
Spencer
@@ -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` |
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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
|
||||
|
||||
+2
-2
@@ -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)
|
||||
"
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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"]|\\\\")',
|
||||
),
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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":"<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":"<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}`);
|
||||
}
|
||||
|
||||
@@ -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"'),
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
@@ -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": {
|
||||
|
||||
Reference in New Issue
Block a user