fix(policy): remove unnecessary escapeRegex from pattern builders#21921
fix(policy): remove unnecessary escapeRegex from pattern builders#21921mattKorwel merged 3 commits intomainfrom
Conversation
spencer426
force-pushed
the
fix/policy-utils-followup-20361
branch
from
2a5f196 to
f6e2be2
Compare
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the policy management system by introducing granular control over where policy rules are saved (user vs. workspace) and automating the default selection for persistent approvals in trusted environments. It also significantly enhances the precision of generated policy rules for various tools by narrowing them to specific arguments, thereby bolstering security. The changes include refactoring regex pattern building for clarity and correctness, and introducing a new category of "sensitive tools" that require more specific policy definitions. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Size Change: -4 B (0%) Total Size: 26.5 MB ℹ️ View Unchanged
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new setting security.autoAddToPolicyByDefault to make "Allow for all future sessions" the default for low-risk tools in trusted workspaces. It also significantly improves security by narrowing the scope of persisted "always allow" policies for file-system and other sensitive tools to specific files or patterns, rather than the entire tool. This is a great enhancement.
However, the PR title and description are misleading as they only refer to a minor refactoring of regex pattern builders, not the main feature being introduced. Please update them to reflect the actual changes.
I've found a consistency issue in how the file paths are handled when creating these narrowed policy rules. For tools like edit_file, write_file, read_file, and ls, the raw file path from the tool parameters is used to generate the policy's argsPattern. This could lead to inconsistent and less secure policies. The path should be resolved to a canonical path relative to the workspace root before being used in a policy. I've left specific suggestions on the relevant files to address this.
Note: Security Review did not run due to the size of the PR.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/core/src/tools/edit.ts (476-482)
To ensure policy consistency and security, the argsPattern for the policy update should be based on a canonical path relative to the workspace root, rather than the raw file_path from parameters. The this.resolvedPath is already available and should be used here, made relative to the target directory. This prevents potential issues where different but equivalent path strings (e.g., foo.txt vs ./foo.txt) would not match the same policy.
override getPolicyUpdateOptions(
_outcome: ToolConfirmationOutcome,
): PolicyUpdateOptions | undefined {
const relativePath = makeRelative(this.resolvedPath, this.config.getTargetDir());
return {
argsPattern: buildFilePathArgsPattern(relativePath),
};
}
References
- File paths used in security policies must be resolved to their canonical, absolute form to prevent path traversal vulnerabilities and ensure consistent policy application. This aligns with the principle of resolving symbolic links and sanitizing user-provided paths for file system operations, and using a single robust function for path validation.
packages/core/src/tools/ls.ts (129-135)
The argsPattern for the ls tool should be based on a canonical, relative path to ensure policy consistency. Currently, it uses the raw dir_path parameter. I suggest resolving the path and then using that resolved path to generate a relative path for the policy pattern.
override getPolicyUpdateOptions(
_outcome: ToolConfirmationOutcome,
): PolicyUpdateOptions | undefined {
const resolvedPath = path.resolve(
this.config.getTargetDir(),
this.params.dir_path,
);
const relativePath = makeRelative(resolvedPath, this.config.getTargetDir());
return {
argsPattern: buildFilePathArgsPattern(relativePath),
};
}References
- File paths used in security policies must be resolved to their canonical, absolute form to prevent path traversal vulnerabilities and ensure consistent policy application. This aligns with the principle of resolving symbolic links and sanitizing user-provided paths for file system operations, and using a single robust function for path validation.
packages/core/src/tools/read-file.ts (94-100)
To improve policy security and consistency, the argsPattern should be based on the resolved, canonical path of the file, relative to the workspace root. Using the raw this.params.file_path can lead to inconsistencies. Please use this.resolvedPath to create the pattern.
override getPolicyUpdateOptions(
_outcome: ToolConfirmationOutcome,
): PolicyUpdateOptions | undefined {
const relativePath = makeRelative(this.resolvedPath, this.config.getTargetDir());
return {
argsPattern: buildFilePathArgsPattern(relativePath),
};
}
References
- File paths used in security policies must be resolved to their canonical, absolute form to prevent path traversal vulnerabilities and ensure consistent policy application. This aligns with the principle of resolving symbolic links and sanitizing user-provided paths for file system operations, and using a single robust function for path validation.
packages/core/src/tools/write-file.ts (169-175)
For more robust and secure policies, the argsPattern should be generated from a canonical path. Please use this.resolvedPath and make it relative to the workspace root, similar to the suggestion for the edit tool. This ensures that policies are consistent regardless of how the path is specified in the tool call.
override getPolicyUpdateOptions(
_outcome: ToolConfirmationOutcome,
): PolicyUpdateOptions | undefined {
const relativePath = makeRelative(this.resolvedPath, this.config.getTargetDir());
return {
argsPattern: buildFilePathArgsPattern(relativePath),
};
}
References
- File paths used in security policies must be resolved to their canonical, absolute form to prevent path traversal vulnerabilities and ensure consistent policy application. This aligns with the principle of resolving symbolic links and sanitizing user-provided paths for file system operations, and using a single robust function for path validation.
spencer426
force-pushed
the
fix/policy-utils-followup-20361
branch
from
a885e01 to
f6e2be2
Compare
gemini-cli
bot
added
area/enterprise
spencer426
force-pushed
the
fix/policy-utils-followup-20361
branch
from
f6e2be2 to
b24c9b8
Compare
mattKorwel
left a comment
mattKorwel
left a comment
There was a problem hiding this comment.
Review and Manual Test Results
- Intent of the PR: The author intended to remove the escapeRegex wrapping from buildFilePathArgsPattern and buildPatternArgsPattern in packages/core/src/policy/utils.ts, claiming that JSON.stringify already safely escapes paths and patterns.
- Identified Bug: Both the static review and the manual script execution identified a critical flaw in this logic. While JSON.stringify safely escapes strings for valid JSON syntax (e.g., quotes and newlines), it does not escape Regular Expression control
characters (such as ., +, ?, [, ], (, ), etc.). - Manual Execution Findings: A test script verified that without escapeRegex, any file path or pattern containing regex special characters (like src/foo[bar].ts) fails to match its literal string counterpart because the characters are interpreted as regex operators
rather than literal characters.
The changes introduce a critical bug that compromises the security, accuracy, and core logic of the policy engine. The removal of escapeRegex breaks literal string matching for any file path or pattern containing regex special characters.
Happy to talk a bit but my review suggests that the removal here of regex escaping leaves us with a security flaw. is this known and intentional?
|
i see this was an explicit ask i left a comment there as well #20361 (comment) happy to be wrong, will unblock as soon as we clarify. |
- Reverted the removal of `escapeRegex` around `JSON.stringify()` in `buildFilePathArgsPattern` and `buildPatternArgsPattern`. As pointed out in PR review #20361, `escapeRegex` is required to prevent Regular Expression control characters (like `.` or `+` in filenames) from acting as regex wildcards, which could allow overly broad policy matches (e.g. `foo.ts` matching `fooXts`). - Removed the old comment documenting LLM generation. - Added explanatory comments explaining exactly why `escapeRegex` is necessary for safety when matching literal stringified arguments.
spencer426
force-pushed
the
fix/policy-utils-followup-20361
branch
from
b24c9b8 to
38ca55a
Compare
|
@mattKorwel Thank you for the incredible catch! You are absolutely right—while I have updated this PR to restore the Thanks again for the thorough manual review! |
3 participants
Summary
This PR addresses post-merge review comments from PR #20361 regarding the `escapeRegex` helper and documentation within the policy utils.
Details
Related Issues
Follow-up to #20361
How to Validate
```bash
npm run test -w @google/gemini-cli-core -- src/policy/
```
Pre-Merge Checklist