Block auto-approve for sed scripts that run commands or write files - #329216
Merged
Conversation
The sed deny rules only matched flag-based forms (-e/-f/--expression/--file) and s/// flags, so positional script arguments fell through to the allow rule. Extend the deny patterns to cover standalone script commands and mirror the change in the agent host fallback defaults. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 102bf1fe-34f3-4b81-b22f-5677ace2be4d
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 102bf1fe-34f3-4b81-b22f-5677ace2be4d
Contributor
There was a problem hiding this comment.
Pull request overview
Tightens default sed auto-approval rules across workbench and Agent Host.
Changes:
- Adds deny patterns for dangerous positional
sedscripts. - Mirrors rules across both approval implementations.
- Adds regression tests for blocked forms.
Show a summary per file
| File | Description |
|---|---|
commandLineAutoApprover.test.ts |
Tests workbench defaults. |
terminalChatAgentToolsConfiguration.ts |
Extends workbench deny rules. |
commandAutoApprover.test.ts |
Tests Agent Host defaults. |
commandAutoApprover.ts |
Mirrors Agent Host deny rules. |
Review details
Suppressed comments (2)
src/vs/workbench/contrib/terminalContrib/chatAgentTools/common/terminalChatAgentToolsConfiguration.ts:321
- The unquoted rule omits the
/regex/address grammar supported by the quoted rules.sed /pat/e input.txtis a valid shell invocation, and GNU sed's argumentlesseexecutes matching pattern-space contents; this line does not match it, so the genericsedallow rule approves it. Include regex addresses in the unquoted grammar and add a regression case.
// Unquoted positional script form (e.g. `sed 1e id`, `sed w file`)
'/^sed\\b(?:\\s+-\\S+)*\\s+(?:(?:\\d+|\\$)(?:,(?:\\d+|\\$))?)?[erRwW]\\s/': false,
src/vs/platform/agentHost/node/commandAutoApprover.ts:651
- The mirrored unquoted rule omits
/regex/addresses.sed /pat/e input.txtis valid, and GNU sed's argumentlesseexecutes matching pattern-space contents; because this does not match any deny rule, it is approved bysed: true. Include the regex-address grammar here and cover it in the Agent Host test.
// Unquoted positional script form (e.g. `sed 1e id`, `sed w file`)
'/^sed\\b(?:\\s+-\\S+)*\\s+(?:(?:\\d+|\\$)(?:,(?:\\d+|\\$))?)?[erRwW]\\s/': false,
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
anthonykim1
marked this pull request as draft
August 5, 2026 17:28
Capture the opening quote so the closing quote must match it, allow the insignificant whitespace and `!` that sed permits around an address, and accept regex addresses in the unquoted script form. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 102bf1fe-34f3-4b81-b22f-5677ace2be4d
anthonykim1
marked this pull request as ready for review
August 5, 2026 17:41
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (4)
src/vs/workbench/contrib/terminalContrib/chatAgentTools/common/terminalChatAgentToolsConfiguration.ts:323
- This quote/address scan still has valid bypasses.
sed "s/a/\\\"/;e id" filestops scanning at the escaped", andsed '/x/p;//e id' fileuses sed's valid empty//address, which this+-based address branch rejects; both then fall through tosed: true. Consume escaped characters before checking the closing delimiter, permit the empty-regex address where a prior expression can establish it, and add both regressions.
'/^sed\\b(?:\\s+-\\S+)*\\s+([\'"])(?:(?!\\1).)*[;{]\\s*(?:(?:\\d+|\\$|\\/(?:\\\\.|[^\\/])+\\/)(?:\\s*,\\s*(?:\\d+|\\$|\\/(?:\\\\.|[^\\/])+\\/))?)?\\s*!?\\s*[erRwW](?:\\s|\\1|[;}])/': false,
src/vs/platform/agentHost/node/commandAutoApprover.ts:651
- This quote/address scan still has valid bypasses.
sed "s/a/\\\"/;e id" filestops scanning at the escaped", andsed '/x/p;//e id' fileuses sed's valid empty//address, which this+-based address branch rejects; both then fall through tosed: true. Consume escaped characters before checking the closing delimiter, permit the empty-regex address where a prior expression can establish it, and add both regressions.
'/^sed\\b(?:\\s+-\\S+)*\\s+([\'"])(?:(?!\\1).)*[;{]\\s*(?:(?:\\d+|\\$|\\/(?:\\\\.|[^\\/])+\\/)(?:\\s*,\\s*(?:\\d+|\\$|\\/(?:\\\\.|[^\\/])+\\/))?)?\\s*!?\\s*[erRwW](?:\\s|\\1|[;}])/': false,
src/vs/workbench/contrib/terminalContrib/chatAgentTools/common/terminalChatAgentToolsConfiguration.ts:325
- This still auto-approves the valid GNU sed script
sed e(and therefore a pipeline such asprintf 'id\n' | sed e). With no command argument,eexecutes the current pattern space, but the unquoted matcher requires trailing whitespace, so the subcommand falls through tosed: true. Treat end-of-command as a valid boundary and add a regression for the no-argumenteform.
'/^sed\\b(?:\\s+-\\S+)*\\s+(?:(?:\\d+|\\$|\\/(?:\\\\.|[^\\/])+\\/)(?:\\s*,\\s*(?:\\d+|\\$|\\/(?:\\\\.|[^\\/])+\\/))?)?\\s*!?\\s*[erRwW]\\s/': false,
src/vs/platform/agentHost/node/commandAutoApprover.ts:653
- This still auto-approves the valid GNU sed script
sed e(and therefore a pipeline such asprintf 'id\n' | sed e). With no command argument,eexecutes the current pattern space, but the unquoted matcher requires trailing whitespace, so the subcommand falls through tosed: true. Treat end-of-command as a valid boundary and add a regression for the no-argumenteform.
'/^sed\\b(?:\\s+-\\S+)*\\s+(?:(?:\\d+|\\$|\\/(?:\\\\.|[^\\/])+\\/)(?:\\s*,\\s*(?:\\d+|\\$|\\/(?:\\\\.|[^\\/])+\\/))?)?\\s*!?\\s*[erRwW]\\s/': false,
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
anthonykim1
marked this pull request as draft
August 5, 2026 17:48
Skip operands consumed by line-length options, keep escaped quotes inside the script scan, allow empty regex addresses, and recognize commands at end of input. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 102bf1fe-34f3-4b81-b22f-5677ace2be4d
anthonykim1
marked this pull request as ready for review
August 5, 2026 18:02
anthonykim1
enabled auto-merge (squash)
August 5, 2026 18:02
roblourens
approved these changes
Aug 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of: #329218
seddeny rules to cover standalonee,r,R,w, andWcommands supplied as positional scripts.;, or inside{ ... }, including line,$,/regex/, and repeated//addresses.!negation, and end-of-command boundaries accepted by sed.-l/--line-lengthso the positional script is still inspected.sed 's/foo/bar/g' file.txt,sed -n '1,10p' file.txt, andsed '/w/d' file.txt.The patterns intentionally fail safe: a literal
;eor{einside a replacement string asks for confirmation instead of auto-approving. A shared script analyzer can remove that conservative match and cover additional sed grammar without duplicating more regex logic; that follow-up is tracked in #329218.Inspirations from:
findandrgentries.seddeny rules interminalChatAgentToolsConfiguration.tsand their Agent Host copy incommandAutoApprover.ts.gitAutoApproveRules.SedFileWriteParser, which is already shared by both surfaces and applied in_matchSubCommands.