From f090736ebcaf52cefd07a600ba59ca9b3fba844c Mon Sep 17 00:00:00 2001 From: Spencer Date: Wed, 11 Mar 2026 22:26:21 -0400 Subject: [PATCH] fix(core): secure argsPattern and revert WEB_FETCH_TOOL_NAME escalation (#22104) Co-authored-by: Taylor Mullen --- .../core/src/core/contentGenerator.test.ts | 2 + packages/core/src/policy/policies/plan.toml | 2 +- packages/core/src/policy/stable-stringify.ts | 27 ++++++++++--- packages/core/src/policy/utils.ts | 40 +++++++++++++++---- packages/core/src/scheduler/policy.test.ts | 3 +- packages/core/src/tools/ls.ts | 4 +- packages/core/src/tools/read-many-files.ts | 6 +-- packages/core/src/tools/web-fetch.ts | 9 ++--- 8 files changed, 67 insertions(+), 26 deletions(-) diff --git a/packages/core/src/core/contentGenerator.test.ts b/packages/core/src/core/contentGenerator.test.ts index c5dcc6e22a..0d470ec934 100644 --- a/packages/core/src/core/contentGenerator.test.ts +++ b/packages/core/src/core/contentGenerator.test.ts @@ -505,6 +505,8 @@ describe('createContentGenerator', () => { }); it('should not include baseUrl in httpOptions when GOOGLE_GEMINI_BASE_URL is not set', async () => { + vi.stubEnv('GOOGLE_GEMINI_BASE_URL', ''); + const mockConfig = { getModel: vi.fn().mockReturnValue('gemini-pro'), getProxy: vi.fn().mockReturnValue(undefined), diff --git a/packages/core/src/policy/policies/plan.toml b/packages/core/src/policy/policies/plan.toml index 86f6554de5..f7e59c5049 100644 --- a/packages/core/src/policy/policies/plan.toml +++ b/packages/core/src/policy/policies/plan.toml @@ -98,7 +98,7 @@ toolName = ["write_file", "replace"] decision = "allow" priority = 70 modes = ["plan"] -argsPattern = "\"file_path\":\"[^\"]+[\\\\/]+\\.gemini[\\\\/]+tmp[\\\\/]+[\\w-]+[\\\\/]+[\\w-]+[\\\\/]+plans[\\\\/]+[\\w-]+\\.md\"" +argsPattern = "\\x00\"file_path\":\"[^\"]+[\\\\/]+\\.gemini[\\\\/]+tmp[\\\\/]+[\\w-]+[\\\\/]+[\\w-]+[\\\\/]+plans[\\\\/]+[\\w-]+\\.md\"\\x00" # Explicitly Deny other write operations in Plan mode with a clear message. [[rule]] diff --git a/packages/core/src/policy/stable-stringify.ts b/packages/core/src/policy/stable-stringify.ts index 8925bc5304..ba9485dbbc 100644 --- a/packages/core/src/policy/stable-stringify.ts +++ b/packages/core/src/policy/stable-stringify.ts @@ -57,7 +57,11 @@ * // Returns: '{"safe":"data"}' */ export function stableStringify(obj: unknown): string { - const stringify = (currentObj: unknown, ancestors: Set): string => { + const stringify = ( + currentObj: unknown, + ancestors: Set, + isTopLevel = false, + ): string => { // Handle primitives and null if (currentObj === undefined) { return 'null'; // undefined in arrays becomes null in JSON @@ -89,7 +93,10 @@ export function stableStringify(obj: unknown): string { if (jsonValue === null) { return 'null'; } - return stringify(jsonValue, ancestors); + // The result of toJSON is effectively a new object graph, but it + // takes the place of the current node, so we preserve the top-level + // status of the current node. + return stringify(jsonValue, ancestors, isTopLevel); } catch { // If toJSON throws, treat as a regular object } @@ -101,7 +108,7 @@ export function stableStringify(obj: unknown): string { if (item === undefined || typeof item === 'function') { return 'null'; } - return stringify(item, ancestors); + return stringify(item, ancestors, false); }); return '[' + items.join(',') + ']'; } @@ -115,7 +122,17 @@ export function stableStringify(obj: unknown): string { const value = (currentObj as Record)[key]; // Skip undefined and function values in objects (per JSON spec) if (value !== undefined && typeof value !== 'function') { - pairs.push(JSON.stringify(key) + ':' + stringify(value, ancestors)); + let pairStr = + JSON.stringify(key) + ':' + stringify(value, ancestors, false); + + if (isTopLevel) { + // We use a null byte (\0) to denote structural boundaries. + // This is safe because any literal \0 in the user's data will + // be escaped by JSON.stringify into "\u0000" before reaching here. + pairStr = '\0' + pairStr + '\0'; + } + + pairs.push(pairStr); } } @@ -125,5 +142,5 @@ export function stableStringify(obj: unknown): string { } }; - return stringify(obj, new Set()); + return stringify(obj, new Set(), true); } diff --git a/packages/core/src/policy/utils.ts b/packages/core/src/policy/utils.ts index f16baa6c0f..3c7bd4d16b 100644 --- a/packages/core/src/policy/utils.ts +++ b/packages/core/src/policy/utils.ts @@ -89,6 +89,25 @@ export function buildArgsPatterns( return [argsPattern]; } +/** + * Builds a regex pattern to match a specific parameter and value in tool arguments. + * This is used to narrow tool approvals to specific parameters. + * + * @param paramName The name of the parameter. + * @param value The value to match. + * @returns A regex string that matches "": in a JSON string. + */ +export function buildParamArgsPattern( + paramName: string, + value: unknown, +): string { + const encodedValue = JSON.stringify(value); + // We wrap the JSON string in escapeRegex and prepend/append \\0 to explicitly + // match top-level JSON properties generated by stableStringify, preventing + // argument injection bypass attacks. + return `\\\\0${escapeRegex(`"${paramName}":${encodedValue}`)}\\\\0`; +} + /** * Builds a regex pattern to match a specific file path in tool arguments. * This is used to narrow tool approvals for edit tools to specific files. @@ -97,11 +116,18 @@ export function buildArgsPatterns( * @returns A regex string that matches "file_path":"" in a JSON string. */ export function buildFilePathArgsPattern(filePath: string): string { - const encodedPath = JSON.stringify(filePath); - // We must wrap the JSON string in escapeRegex to ensure regex control characters - // (like '.' in file extensions) are treated as literals, preventing overly broad - // matches (e.g. 'foo.ts' matching 'fooXts'). - return escapeRegex(`"file_path":${encodedPath}`); + return buildParamArgsPattern('file_path', filePath); +} + +/** + * Builds a regex pattern to match a specific directory path in tool arguments. + * This is used to narrow tool approvals for list_directory tool. + * + * @param dirPath The path to the directory. + * @returns A regex string that matches "dir_path":"" in a JSON string. + */ +export function buildDirPathArgsPattern(dirPath: string): string { + return buildParamArgsPattern('dir_path', dirPath); } /** @@ -112,7 +138,5 @@ export function buildFilePathArgsPattern(filePath: string): string { * @returns A regex string that matches "pattern":"" in a JSON string. */ export function buildPatternArgsPattern(pattern: string): string { - const encodedPattern = JSON.stringify(pattern); - // We use escapeRegex to ensure regex control characters are treated as literals. - return escapeRegex(`"pattern":${encodedPattern}`); + return buildParamArgsPattern('pattern', pattern); } diff --git a/packages/core/src/scheduler/policy.test.ts b/packages/core/src/scheduler/policy.test.ts index 796b9f2803..c87456da67 100644 --- a/packages/core/src/scheduler/policy.test.ts +++ b/packages/core/src/scheduler/policy.test.ts @@ -660,7 +660,8 @@ describe('policy.ts', () => { expect(mockMessageBus.publish).toHaveBeenCalledWith( expect.objectContaining({ toolName: 'write_file', - argsPattern: escapeRegex('"file_path":"src/foo.ts"'), + argsPattern: + '\\\\0' + escapeRegex('"file_path":"src/foo.ts"') + '\\\\0', }), ); }); diff --git a/packages/core/src/tools/ls.ts b/packages/core/src/tools/ls.ts index 1e2d1cccf8..a6850ed825 100644 --- a/packages/core/src/tools/ls.ts +++ b/packages/core/src/tools/ls.ts @@ -21,7 +21,7 @@ import type { Config } from '../config/config.js'; import { DEFAULT_FILE_FILTERING_OPTIONS } from '../config/constants.js'; import { ToolErrorType } from './tool-error.js'; import { LS_TOOL_NAME } from './tool-names.js'; -import { buildFilePathArgsPattern } from '../policy/utils.js'; +import { buildDirPathArgsPattern } from '../policy/utils.js'; import { debugLogger } from '../utils/debugLogger.js'; import { LS_DEFINITION } from './definitions/coreTools.js'; import { resolveToolDeclaration } from './definitions/resolver.js'; @@ -130,7 +130,7 @@ class LSToolInvocation extends BaseToolInvocation { _outcome: ToolConfirmationOutcome, ): PolicyUpdateOptions | undefined { return { - argsPattern: buildFilePathArgsPattern(this.params.dir_path), + argsPattern: buildDirPathArgsPattern(this.params.dir_path), }; } diff --git a/packages/core/src/tools/read-many-files.ts b/packages/core/src/tools/read-many-files.ts index 4a2ae9a4c0..c297f95ae8 100644 --- a/packages/core/src/tools/read-many-files.ts +++ b/packages/core/src/tools/read-many-files.ts @@ -18,7 +18,7 @@ import { getErrorMessage } from '../utils/errors.js'; import * as fsPromises from 'node:fs/promises'; import * as path from 'node:path'; import { glob, escape } from 'glob'; -import { buildPatternArgsPattern } from '../policy/utils.js'; +import { buildParamArgsPattern } from '../policy/utils.js'; import { detectFileType, processSingleFileContent, @@ -161,10 +161,8 @@ ${finalExclusionPatternsForDescription override getPolicyUpdateOptions( _outcome: ToolConfirmationOutcome, ): PolicyUpdateOptions | undefined { - // We join the include patterns to match the JSON stringified arguments. - // buildPatternArgsPattern handles JSON stringification. return { - argsPattern: buildPatternArgsPattern(JSON.stringify(this.params.include)), + argsPattern: buildParamArgsPattern('include', this.params.include), }; } diff --git a/packages/core/src/tools/web-fetch.ts b/packages/core/src/tools/web-fetch.ts index e4d9ebc36f..7d16fb1d76 100644 --- a/packages/core/src/tools/web-fetch.ts +++ b/packages/core/src/tools/web-fetch.ts @@ -14,7 +14,7 @@ import { type ToolConfirmationOutcome, type PolicyUpdateOptions, } from './tools.js'; -import { buildPatternArgsPattern } from '../policy/utils.js'; +import { buildParamArgsPattern } from '../policy/utils.js'; import type { MessageBus } from '../confirmation-bus/message-bus.js'; import { ToolErrorType } from './tool-error.js'; import { getErrorMessage } from '../utils/errors.js'; @@ -328,12 +328,11 @@ ${textContent} ): PolicyUpdateOptions | undefined { if (this.params.url) { return { - argsPattern: buildPatternArgsPattern(this.params.url), + argsPattern: buildParamArgsPattern('url', this.params.url), }; - } - if (this.params.prompt) { + } else if (this.params.prompt) { return { - argsPattern: buildPatternArgsPattern(this.params.prompt), + argsPattern: buildParamArgsPattern('prompt', this.params.prompt), }; } return undefined;