Add a global ignore list for read operations #4230
Conversation
🦋 Changeset detectedLatest commit: 646652d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
I'm having a hard time testing this locally, but was hoping to get some input from you @RSO and @catrielmuller. The idea here is to protect users from accidentally having sensitive content read and sent to an LLM. With this feature, common secrets are excluded even if a user has not yet created a .kilocodeignore file. Also cc @LigiaZ for input as well. |
kevinvandijk
left a comment
kevinvandijk
left a comment
There was a problem hiding this comment.
@i’ll review more on actual content later but please make sure to add appropriate kilocode_change start and end markers on adjusted sections
| </div> | ||
| </Button> | ||
| ))} | ||
| </div> |
There was a problem hiding this comment.
Can you wrap this + the handler above into its own component to reduce the diff with merges but also to keep the AutoApproveSettings component smaller?
| export class RooIgnoreController { | ||
| private cwd: string | ||
| private ignoreInstance: Ignore | ||
| private globalIgnoreInstance: Ignore |
There was a problem hiding this comment.
All changes in this file need // kilocode_change comments
| MIN_CHECKPOINT_TIMEOUT_SECONDS, | ||
| TOOL_PROTOCOL, | ||
| ToolProtocol, | ||
| DEFAULT_GLOBALLY_IGNORED_FILES, |
There was a problem hiding this comment.
All changes in this file need // kilocode_change as well
Add kilocode_change start and end tags Co-authored-by: Remon Oldenbeuving <[email protected]>
|
Thanks for the work on this update. Adding a global ignore list is a meaningful step toward safer defaults.
It may also be helpful to revisit how ignore logic interacts with Lastly, strengthening workspace boundary enforcement would further reduce potential for unintended access. Happy to provide additional context privately if helpful. |
Thanks for the feedback @vman00, I will review it in more detail a bit later but just quickly on this point there are two other open PR's that may help address this that change the default settings of auto-approve: Extension (#4228), CLI (#4186) Please let me know if you think this would address this concern, or if there are other protections you would be interested in seeing. |
Co-authored-by: Remon Oldenbeuving <[email protected]>
Co-authored-by: Kevin van Dijk <[email protected]>
![kiloconnect[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No New Issues Found
11 files reviewed | Confidence: 95% | Recommendation: Merge
Summary
This PR implements a solid security feature that provides default protection for sensitive files (.env*, *.key, *.pem, SSH keys, etc.) even without a .kilocodeignore file present.
Key Implementation Details:
- Global patterns are checked FIRST before
.kilocodeignorepatterns, ensuring security cannot be bypassed - The
ignorelibrary is used for gitignore-style pattern matching - Async initialization uses defaults during the brief window before settings load
- Comprehensive test coverage (+193 lines) covering all scenarios
Review Details
Files Reviewed:
packages/types/src/global-settings.ts- Default patterns constantsrc/core/ignore/RooIgnoreController.ts- Core implementationsrc/core/task/Task.ts- Initialization logicsrc/core/webview/ClineProvider.ts- State managementsrc/core/ignore/__tests__/RooIgnoreController.spec.ts- Test coveragewebview-ui/src/components/settings/AutoApproveSettings.tsx- UI integrationsrc/shared/ExtensionMessage.ts- Type definitionsapps/kilocode-docs/docs/features/auto-approving-actions.md- Documentation
Checked: Security vulnerabilities, race conditions, pattern matching correctness, test coverage, UI integration
Previous Review Comments: All addressed (kilocode_change markers, documentation tweaks, changeset version)
There was a problem hiding this comment.
⚠️ 1 Issue Found
| Severity | Issue | Location |
|---|---|---|
| WARNING | Global ignore patterns bypassed in terminal commands | RooIgnoreController.ts:140-143 |
Recommendation: Address the warning before merge to ensure terminal commands respect global ignore patterns.
Review Details (11 files)
Files reviewed:
.changeset/pretty-pants-end.md✓apps/kilocode-docs/docs/features/auto-approving-actions.md✓packages/types/src/global-settings.ts✓src/core/ignore/RooIgnoreController.ts(1 issue)src/core/ignore/__tests__/RooIgnoreController.spec.ts✓src/core/task/Task.ts✓src/core/webview/ClineProvider.ts✓src/shared/ExtensionMessage.ts✓webview-ui/src/components/settings/AutoApproveSettings.tsx✓webview-ui/src/components/settings/SettingsView.tsx✓webview-ui/src/i18n/locales/en/settings.json✓
Checked: Security vulnerabilities, bug detection, code patterns, error handling
Positive aspects:
- Good security feature addressing a real gap in sensitive file protection
- Global patterns correctly take priority over
.kilocodeignoreinvalidateAccess() - Comprehensive test coverage for the new functionality
- Default patterns cover common sensitive file types (env files, keys, certificates, SSH keys)
- Clean async initialization pattern with fallback to defaults
| } | ||
| } // kilocode_change end | ||
|
|
||
| /** |
There was a problem hiding this comment.
validateCommand() returns early when no .kilocodeignore exists, allowing commands like cat .env to bypass global ignore patterns. Change line 140 to also check this.globallyIgnoredFiles.length === 0 before returning, or remove this early return and let validateAccess() handle both cases.
Context
Kilo Code supports a .kilocodeignore file, to prevent secrets and other sensitive content from being read and exposed to an LLM. However this must be created specifically for each project, and in the event it doesn't exist, read operations are allowed for all file types.
.gitignore can help in these situations, but it is not comprehensive as it does not get applied (per our documentation) for read operations, only for certain directory list operations.
This introduces a new concept to add a global ignore list which works even without the presence of a .kilocodeignore file, so that common secrets and other sensitive file types are ignored by default.
Implementation
Adds a new option to the auto-approve list, which shows when read is enabled, and has supports a list of glob syntax similar to other settings. This is then checked to prevent auto-approved read or write operations.
Screenshots
How to Test
Get in Touch