From fd3242f4ccddd1514a701a3c40b03bacaeae7bec Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Mon, 3 Aug 2026 19:10:06 -0700 Subject: [PATCH 1/6] Require confirmation for sed in-place auto-approval Add a shared semantic sed analyzer and apply it before Agent Host terminal allow rules so in-place and runtime-resolved option forms cannot auto-approve. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bda42f98-e73f-417f-b7e6-03cff9e2f604 --- .../agentHost/node/commandAutoApprover.ts | 4 + .../test/node/commandAutoApprover.test.ts | 33 ++++++ .../test/node/sessionPermissions.test.ts | 22 ++++ .../terminal/common/sedCommandAnalyzer.ts | 107 ++++++++++++++++++ .../test/common/sedCommandAnalyzer.test.ts | 72 ++++++++++++ 5 files changed, 238 insertions(+) create mode 100644 src/vs/platform/terminal/common/sedCommandAnalyzer.ts create mode 100644 src/vs/platform/terminal/test/common/sedCommandAnalyzer.test.ts diff --git a/src/vs/platform/agentHost/node/commandAutoApprover.ts b/src/vs/platform/agentHost/node/commandAutoApprover.ts index 6c713d18688cdb..90b8555ce6636c 100644 --- a/src/vs/platform/agentHost/node/commandAutoApprover.ts +++ b/src/vs/platform/agentHost/node/commandAutoApprover.ts @@ -11,6 +11,7 @@ import { escapeRegExpCharacters, regExpLeadsToEndlessLoop } from '../../../base/ import { URI } from '../../../base/common/uri.js'; import { getAppNodeModulesPath } from './appNodeModules.js'; import { ILogService } from '../../log/common/log.js'; +import { analyzeSedCommand, SedCommandAnalysis } from '../../terminal/common/sedCommandAnalyzer.js'; import type { AgentHostTerminalAutoApproveRuleValue, AgentHostTerminalAutoApproveRules } from '../common/agentHostSchema.js'; /** @@ -261,6 +262,9 @@ export class CommandAutoApprover extends Disposable { private _matchSubCommands(subCommands: string[], rules: IAutoApproveRules, isPowerShell: boolean): CommandApprovalResult { let allApproved = true; for (const subCommand of subCommands) { + if (analyzeSedCommand(subCommand) === SedCommandAnalysis.RequiresConfirmation) { + return 'denied'; + } // Deny transient env var assignments if (transientEnvVarRegex.test(subCommand)) { return 'denied'; diff --git a/src/vs/platform/agentHost/test/node/commandAutoApprover.test.ts b/src/vs/platform/agentHost/test/node/commandAutoApprover.test.ts index e0aa454b3aa058..ed04cf47f5f341 100644 --- a/src/vs/platform/agentHost/test/node/commandAutoApprover.test.ts +++ b/src/vs/platform/agentHost/test/node/commandAutoApprover.test.ts @@ -86,6 +86,39 @@ suite('CommandAutoApprover', () => { assert.strictEqual(approver.shouldAutoApprove('sed --expression "s/foo/bar/"'), 'denied'); }); + test('requires confirmation for sed in-place and dynamic option forms', () => { + const commands = [ + 'sed -i "s/foo/bar/" file.txt', + 'sed -I "s/foo/bar/" file.txt', + 'sed -ni "s/foo/bar/" file.txt', + 'sed -i.bak "s/foo/bar/" file.txt', + 'sed --in-place "s/foo/bar/" file.txt', + 'sed --in-plac "s/foo/bar/" file.txt', + 'sed.exe -i "s/foo/bar/" file.txt', + 'sed -\\i "s/foo/bar/" file.txt', + 'sed "$SED_OPTIONS" "s/foo/bar/" file.txt', + 'sed "s/foo/bar/" "$(echo --in-place)" file.txt', + 'sed${PATH:+} -i "s/foo/bar/" file.txt', + 'sed${PATH:+} "s/foo/bar/" file.txt', + ]; + assert.deepStrictEqual( + commands.map(commandLine => approver.shouldAutoApprove(commandLine)), + commands.map(() => 'denied'), + ); + }); + + test('sed safety gate cannot be overridden by allow rules', () => { + const commandLine = 'sed -i "s/foo/bar/" file.txt'; + const autoApproveRules = { + sed: true, + '/^sed -i "s\\/foo\\/bar\\/" file\\.txt$/': { approve: true, matchCommandLine: true }, + }; + assert.deepStrictEqual( + approver.evaluate(commandLine, { autoApproveRules }), + { result: 'denied', autoApproveRuleResolvable: false }, + ); + }); + // npm/package managers test('approves allowed npm commands', () => { assert.strictEqual(approver.shouldAutoApprove('npm ci'), 'approved'); diff --git a/src/vs/platform/agentHost/test/node/sessionPermissions.test.ts b/src/vs/platform/agentHost/test/node/sessionPermissions.test.ts index 73df9e4774079c..9051b9518bcfbc 100644 --- a/src/vs/platform/agentHost/test/node/sessionPermissions.test.ts +++ b/src/vs/platform/agentHost/test/node/sessionPermissions.test.ts @@ -239,6 +239,28 @@ suite('SessionPermissionManager', () => { assert.strictEqual(result, ToolCallConfirmationReason.NotNeeded); }); + test('requires confirmation for sed in-place edits regardless of destination', async () => { + const commands = [ + 'sed -i "s/foo/bar/" file.txt', + `sed --in-place "s/foo/bar/" ${join(outsideDir, 'file.txt')}`, + 'sed "$SED_OPTIONS" "s/foo/bar/" file.txt', + ]; + const approvals = []; + const ruleResolvable = []; + for (const commandLine of commands) { + const event = shellEvent(commandLine, 'bash'); + approvals.push(await permissions.getAutoApproval(event, sessionUri)); + ruleResolvable.push(permissions.isAutoApproveRuleResolvable(event, sessionUri)); + } + assert.deepStrictEqual({ + approvals, + ruleResolvable, + }, { + approvals: commands.map(() => undefined), + ruleResolvable: commands.map(() => false), + }); + }); + test('uses forwarded terminal auto-approve rules as the source of truth over fallback defaults', async () => { configService.updateRootConfig({ [AgentHostTerminalAutoApproveRulesConfigKey]: {} }); diff --git a/src/vs/platform/terminal/common/sedCommandAnalyzer.ts b/src/vs/platform/terminal/common/sedCommandAnalyzer.ts new file mode 100644 index 00000000000000..d77e7504f5f30b --- /dev/null +++ b/src/vs/platform/terminal/common/sedCommandAnalyzer.ts @@ -0,0 +1,107 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +export const enum SedCommandAnalysis { + Safe, + RequiresConfirmation, +} + +interface IShellWord { + readonly value: string; + readonly hasRuntimeExpansion: boolean; +} + +const inPlaceLongOption = '--in-place'; + +export function analyzeSedCommand(commandText: string): SedCommandAnalysis { + const words = tokenizeCommand(commandText); + const executable = words[0]; + if (!executable || !/^sed\b/.test(executable.value)) { + return SedCommandAnalysis.Safe; + } + if (executable.hasRuntimeExpansion) { + return SedCommandAnalysis.RequiresConfirmation; + } + + for (const word of words.slice(1)) { + if (word.value === '--') { + break; + } + if (word.hasRuntimeExpansion) { + return SedCommandAnalysis.RequiresConfirmation; + } + if (word.value.startsWith('--')) { + const optionName = word.value.split('=', 1)[0]; + if (optionName.length >= 3 && inPlaceLongOption.startsWith(optionName)) { + return SedCommandAnalysis.RequiresConfirmation; + } + continue; + } + if (word.value.startsWith('-') && /[iI]/.test(word.value.slice(1))) { + return SedCommandAnalysis.RequiresConfirmation; + } + } + return SedCommandAnalysis.Safe; +} + +function tokenizeCommand(commandText: string): IShellWord[] { + const words: IShellWord[] = []; + let value = ''; + let hasRuntimeExpansion = false; + let quote: '\'' | '"' | undefined; + let escaping = false; + let wordStarted = false; + + const pushWord = () => { + words.push({ value, hasRuntimeExpansion }); + value = ''; + hasRuntimeExpansion = false; + wordStarted = false; + }; + + for (const char of commandText) { + if (escaping) { + wordStarted = true; + if (char !== '\n') { + value += char; + } + escaping = false; + continue; + } + if (char === '\\' && quote !== '\'') { + wordStarted = true; + escaping = true; + continue; + } + if (char === quote) { + quote = undefined; + continue; + } + if (!quote && (char === '\'' || char === '"')) { + wordStarted = true; + quote = char; + continue; + } + if (!quote && /\s/.test(char)) { + if (wordStarted) { + pushWord(); + } + continue; + } + if (quote !== '\'' && (char === '$' || char === '`' || (!quote && /[*?[{()}]/.test(char)))) { + hasRuntimeExpansion = true; + } + wordStarted = true; + value += char; + } + + if (wordStarted) { + if (quote || escaping) { + hasRuntimeExpansion = true; + } + pushWord(); + } + return words; +} diff --git a/src/vs/platform/terminal/test/common/sedCommandAnalyzer.test.ts b/src/vs/platform/terminal/test/common/sedCommandAnalyzer.test.ts new file mode 100644 index 00000000000000..79f7836d0f9da8 --- /dev/null +++ b/src/vs/platform/terminal/test/common/sedCommandAnalyzer.test.ts @@ -0,0 +1,72 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from 'assert'; +import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js'; +import { analyzeSedCommand, SedCommandAnalysis } from '../../common/sedCommandAnalyzer.js'; + +suite('analyzeSedCommand', () => { + + ensureNoDisposablesAreLeakedInTestSuite(); + + test('allows literal non-in-place commands', () => { + const commands = [ + 'echo sed -i file.txt', + 'sed "s/foo/bar/" file.txt', + 'sed -n "s/foo/bar/p" file.txt', + 'sed -E "s/(foo)/bar/" file.txt', + 'sed --quiet "s/foo/bar/p" file.txt', + 'sed --sandbox "s/foo/bar/" file.txt', + 'sed -- "$SED_OPTIONS" file.txt', + ]; + + assert.deepStrictEqual( + commands.map(analyzeSedCommand), + commands.map(() => SedCommandAnalysis.Safe), + ); + }); + + test('requires confirmation for semantic in-place options', () => { + const commands = [ + 'sed -i "s/foo/bar/" file.txt', + 'sed -I "s/foo/bar/" file.txt', + 'sed -ni "s/foo/bar/" file.txt', + 'sed -n -i "s/foo/bar/" file.txt', + 'sed -i.bak "s/foo/bar/" file.txt', + 'sed -i \'\' "s/foo/bar/" file.txt', + 'sed --in-place "s/foo/bar/" file.txt', + 'sed --in-place=.bak "s/foo/bar/" file.txt', + 'sed --in-plac "s/foo/bar/" file.txt', + 'sed.exe -i "s/foo/bar/" file.txt', + 'sed "-i" "s/foo/bar/" file.txt', + 'sed -\\i "s/foo/bar/" file.txt', + 'sed "s/foo/bar/" -inside.txt', + 'sed -i\'../outside/*\' "s/foo/bar/" file.txt', + 'sed --follow-symlinks -i "s/foo/bar/" link.txt', + ]; + + assert.deepStrictEqual( + commands.map(analyzeSedCommand), + commands.map(() => SedCommandAnalysis.RequiresConfirmation), + ); + }); + + test('requires confirmation for runtime-resolved option words', () => { + const commands = [ + 'sed "$SED_OPTIONS" "s/foo/bar/" file.txt', + 'sed "$(echo --in-place)" "s/foo/bar/" file.txt', + 'sed "s/foo/bar/" "$SED_OPTIONS" file.txt', + 'sed "s/foo/bar/" "$(echo --in-place)" file.txt', + 'sed -i "s/foo/bar/" *.txt', + 'sed${PATH:+} -i "s/foo/bar/" file.txt', + 'sed${PATH:+} "s/foo/bar/" file.txt', + ]; + + assert.deepStrictEqual( + commands.map(analyzeSedCommand), + commands.map(() => SedCommandAnalysis.RequiresConfirmation), + ); + }); +}); From 3b8ee05e104907315cd99471d341949962579798 Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Mon, 3 Aug 2026 19:55:12 -0700 Subject: [PATCH 2/6] Share sed in-place destination analysis Use one GNU/BSD-aware analyzer in workbench and Agent Host, applying existing destination policies to static writes and failing closed otherwise. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bda42f98-e73f-417f-b7e6-03cff9e2f604 --- .../agentHost/node/commandAutoApprover.ts | 19 +- .../test/node/commandAutoApprover.test.ts | 51 +++- .../test/node/sessionPermissions.test.ts | 18 +- .../terminal/common/sedCommandAnalyzer.ts | 233 +++++++++++++++--- .../test/common/sedCommandAnalyzer.test.ts | 81 +++++- .../commandParsers/commandFileWriteParser.ts | 5 +- .../commandParsers/sedFileWriteParser.ts | 192 +-------------- .../commandLineFileWriteAnalyzer.ts | 18 +- .../browser/treeSitterCommandParser.ts | 4 +- .../commandLineFileWriteAnalyzer.test.ts | 10 +- 10 files changed, 378 insertions(+), 253 deletions(-) diff --git a/src/vs/platform/agentHost/node/commandAutoApprover.ts b/src/vs/platform/agentHost/node/commandAutoApprover.ts index 90b8555ce6636c..bda8e77ebfdce0 100644 --- a/src/vs/platform/agentHost/node/commandAutoApprover.ts +++ b/src/vs/platform/agentHost/node/commandAutoApprover.ts @@ -11,7 +11,7 @@ import { escapeRegExpCharacters, regExpLeadsToEndlessLoop } from '../../../base/ import { URI } from '../../../base/common/uri.js'; import { getAppNodeModulesPath } from './appNodeModules.js'; import { ILogService } from '../../log/common/log.js'; -import { analyzeSedCommand, SedCommandAnalysis } from '../../terminal/common/sedCommandAnalyzer.js'; +import { analyzeSedCommand } from '../../terminal/common/sedCommandAnalyzer.js'; import type { AgentHostTerminalAutoApproveRuleValue, AgentHostTerminalAutoApproveRules } from '../common/agentHostSchema.js'; /** @@ -246,25 +246,28 @@ export class CommandAutoApprover extends Disposable { return { result: 'noMatch', autoApproveRuleResolvable: false }; } - const hasUnapprovedRedirect = () => parsed.unsafeWriteDests.some(dest => dest === undefined || !options?.isWriteDestApproved?.(dest)); + const sedAnalyses = parsed.subCommands.map(subCommand => analyzeSedCommand(subCommand, isPowerShell ? 'powershell' : 'bash')); + if (sedAnalyses.some(analysis => analysis.kind === 'requiresConfirmation')) { + return { result: 'denied', autoApproveRuleResolvable: false }; + } + const sedWriteDests = sedAnalyses.flatMap(analysis => analysis.kind === 'inPlace' ? analysis.fileWrites : []); + const writeDests = [...parsed.unsafeWriteDests, ...sedWriteDests]; + const hasUnapprovedWriteDest = () => writeDests.some(dest => dest === undefined || !options?.isWriteDestApproved?.(dest)); let result = this._matchSubCommands(parsed.subCommands, rules, isPowerShell); if (result !== 'denied' && this._matchesCommandLineRule(trimmed, rules.allowCommandLineRules)) { result = 'approved'; } - if (result === 'approved' && hasUnapprovedRedirect()) { - this._logService.trace('[CommandAutoApprover] Write redirection to non-approved destination, requiring confirmation'); + if (result === 'approved' && hasUnapprovedWriteDest()) { + this._logService.trace('[CommandAutoApprover] Write to non-approved destination, requiring confirmation'); return { result: 'noMatch', autoApproveRuleResolvable: false }; } - return { result, autoApproveRuleResolvable: result === 'noMatch' && !hasUnapprovedRedirect() }; + return { result, autoApproveRuleResolvable: result === 'noMatch' && !hasUnapprovedWriteDest() }; } private _matchSubCommands(subCommands: string[], rules: IAutoApproveRules, isPowerShell: boolean): CommandApprovalResult { let allApproved = true; for (const subCommand of subCommands) { - if (analyzeSedCommand(subCommand) === SedCommandAnalysis.RequiresConfirmation) { - return 'denied'; - } // Deny transient env var assignments if (transientEnvVarRegex.test(subCommand)) { return 'denied'; diff --git a/src/vs/platform/agentHost/test/node/commandAutoApprover.test.ts b/src/vs/platform/agentHost/test/node/commandAutoApprover.test.ts index ed04cf47f5f341..0e618b79ab14f1 100644 --- a/src/vs/platform/agentHost/test/node/commandAutoApprover.test.ts +++ b/src/vs/platform/agentHost/test/node/commandAutoApprover.test.ts @@ -86,20 +86,52 @@ suite('CommandAutoApprover', () => { assert.strictEqual(approver.shouldAutoApprove('sed --expression "s/foo/bar/"'), 'denied'); }); - test('requires confirmation for sed in-place and dynamic option forms', () => { + test('checks static sed in-place write destinations', () => { + const seen: string[] = []; + const options = { + isWriteDestApproved: (dest: string) => { + seen.push(dest); + return dest === 'file.txt' || dest === 'file.txt.bak'; + }, + }; const commands = [ 'sed -i "s/foo/bar/" file.txt', - 'sed -I "s/foo/bar/" file.txt', + 'sed -I .bak "s/foo/bar/" file.txt', 'sed -ni "s/foo/bar/" file.txt', 'sed -i.bak "s/foo/bar/" file.txt', 'sed --in-place "s/foo/bar/" file.txt', 'sed --in-plac "s/foo/bar/" file.txt', 'sed.exe -i "s/foo/bar/" file.txt', 'sed -\\i "s/foo/bar/" file.txt', + ]; + assert.deepStrictEqual( + commands.map(commandLine => approver.shouldAutoApprove(commandLine, options)), + commands.map(() => 'approved'), + ); + assert.deepStrictEqual(seen, [ + 'file.txt', + 'file.txt', + 'file.txt.bak', + 'file.txt', + 'file.txt', + 'file.txt.bak', + 'file.txt', + 'file.txt', + 'file.txt', + 'file.txt', + ]); + }); + + test('requires confirmation for dynamic or ambiguous sed forms', () => { + const commands = [ 'sed "$SED_OPTIONS" "s/foo/bar/" file.txt', 'sed "s/foo/bar/" "$(echo --in-place)" file.txt', 'sed${PATH:+} -i "s/foo/bar/" file.txt', 'sed${PATH:+} "s/foo/bar/" file.txt', + 'sed --follow-symlinks -i "s/foo/bar/" file.txt', + 'sed --in-place --expr="s/foo/bar/" file.txt', + 'sed -i.bak "-e" $ARGS inside.txt', + 'sed --in-place --file "$SCRIPT" inside.txt', ]; assert.deepStrictEqual( commands.map(commandLine => approver.shouldAutoApprove(commandLine)), @@ -107,16 +139,21 @@ suite('CommandAutoApprover', () => { ); }); - test('sed safety gate cannot be overridden by allow rules', () => { + test('sed write policy cannot be overridden by allow rules', () => { const commandLine = 'sed -i "s/foo/bar/" file.txt'; const autoApproveRules = { sed: true, '/^sed -i "s\\/foo\\/bar\\/" file\\.txt$/': { approve: true, matchCommandLine: true }, }; - assert.deepStrictEqual( - approver.evaluate(commandLine, { autoApproveRules }), - { result: 'denied', autoApproveRuleResolvable: false }, - ); + assert.deepStrictEqual({ + withoutPredicate: approver.evaluate(commandLine, { autoApproveRules }), + rejected: approver.evaluate(commandLine, { autoApproveRules, isWriteDestApproved: () => false }), + accepted: approver.evaluate(commandLine, { autoApproveRules, isWriteDestApproved: () => true }), + }, { + withoutPredicate: { result: 'noMatch', autoApproveRuleResolvable: false }, + rejected: { result: 'noMatch', autoApproveRuleResolvable: false }, + accepted: { result: 'approved', autoApproveRuleResolvable: false }, + }); }); // npm/package managers diff --git a/src/vs/platform/agentHost/test/node/sessionPermissions.test.ts b/src/vs/platform/agentHost/test/node/sessionPermissions.test.ts index 9051b9518bcfbc..2a4a1d48a6b06e 100644 --- a/src/vs/platform/agentHost/test/node/sessionPermissions.test.ts +++ b/src/vs/platform/agentHost/test/node/sessionPermissions.test.ts @@ -239,15 +239,17 @@ suite('SessionPermissionManager', () => { assert.strictEqual(result, ToolCallConfirmationReason.NotNeeded); }); - test('requires confirmation for sed in-place edits regardless of destination', async () => { - const commands = [ - 'sed -i "s/foo/bar/" file.txt', - `sed --in-place "s/foo/bar/" ${join(outsideDir, 'file.txt')}`, - 'sed "$SED_OPTIONS" "s/foo/bar/" file.txt', + test('sed in-place edits use the shell destination policy', async () => { + const cases: [commandLine: string, expected: ToolCallConfirmationReason | undefined][] = [ + ['sed -i "s/foo/bar/" file.txt', ToolCallConfirmationReason.NotNeeded], + ['sed -i.bak "s/foo/bar/" file.txt', ToolCallConfirmationReason.NotNeeded], + [`sed --in-place "s/foo/bar/" ${join(outsideDir, 'file.txt')}`, undefined], + ['sed -i "s/foo/bar/" package.json', undefined], + ['sed "$SED_OPTIONS" "s/foo/bar/" file.txt', undefined], ]; const approvals = []; const ruleResolvable = []; - for (const commandLine of commands) { + for (const [commandLine] of cases) { const event = shellEvent(commandLine, 'bash'); approvals.push(await permissions.getAutoApproval(event, sessionUri)); ruleResolvable.push(permissions.isAutoApproveRuleResolvable(event, sessionUri)); @@ -256,8 +258,8 @@ suite('SessionPermissionManager', () => { approvals, ruleResolvable, }, { - approvals: commands.map(() => undefined), - ruleResolvable: commands.map(() => false), + approvals: cases.map(([, expected]) => expected), + ruleResolvable: cases.map(() => false), }); }); diff --git a/src/vs/platform/terminal/common/sedCommandAnalyzer.ts b/src/vs/platform/terminal/common/sedCommandAnalyzer.ts index d77e7504f5f30b..7fa1a1dde14611 100644 --- a/src/vs/platform/terminal/common/sedCommandAnalyzer.ts +++ b/src/vs/platform/terminal/common/sedCommandAnalyzer.ts @@ -3,50 +3,224 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -export const enum SedCommandAnalysis { - Safe, - RequiresConfirmation, -} +export type SedCommandAnalysis = + | { readonly kind: 'safe' } + | { readonly kind: 'inPlace'; readonly fileWrites: readonly string[] } + | { readonly kind: 'requiresConfirmation' }; interface IShellWord { readonly value: string; readonly hasRuntimeExpansion: boolean; } +interface ISedParseResult { + readonly kind: 'safe' | 'inPlace' | 'invalidInPlace' | 'requiresConfirmation'; + readonly fileWrites?: readonly string[]; +} + +const safe: SedCommandAnalysis = { kind: 'safe' }; +const requiresConfirmation: SedCommandAnalysis = { kind: 'requiresConfirmation' }; const inPlaceLongOption = '--in-place'; -export function analyzeSedCommand(commandText: string): SedCommandAnalysis { - const words = tokenizeCommand(commandText); +export function analyzeSedCommand(commandText: string, shellDialect: 'bash' | 'powershell' = 'bash'): SedCommandAnalysis { + const words = tokenizeCommand(commandText, shellDialect); const executable = words[0]; - if (!executable || !/^sed\b/.test(executable.value)) { - return SedCommandAnalysis.Safe; + if (!executable || !isSedExecutable(executable.value)) { + return safe; } if (executable.hasRuntimeExpansion) { - return SedCommandAnalysis.RequiresConfirmation; + return requiresConfirmation; } - for (const word of words.slice(1)) { - if (word.value === '--') { - break; - } + const results = [parseSedArguments(words.slice(1), 'gnu'), parseSedArguments(words.slice(1), 'bsd')]; + if (results.some(result => result.kind === 'requiresConfirmation')) { + return requiresConfirmation; + } + const inPlaceResults = results.filter((result): result is ISedParseResult & { kind: 'inPlace'; fileWrites: readonly string[] } => result.kind === 'inPlace'); + if (inPlaceResults.length === 0) { + return results.some(result => result.kind === 'invalidInPlace') ? requiresConfirmation : safe; + } + const fileWrites = [...new Set(inPlaceResults.flatMap(result => result.fileWrites))]; + return fileWrites.length > 0 ? { kind: 'inPlace', fileWrites } : requiresConfirmation; +} + +function parseSedArguments(arguments_: readonly IShellWord[], style: 'gnu' | 'bsd'): ISedParseResult { + const operands: string[] = []; + let inPlaceSuffix: string | undefined; + let hasScriptOption = false; + let hasUnknownOption = false; + let hasDynamicOperand = false; + let optionsEnded = false; + + for (let index = 0; index < arguments_.length; index++) { + const word = arguments_[index]; + const argument = word.value; if (word.hasRuntimeExpansion) { - return SedCommandAnalysis.RequiresConfirmation; + if (!optionsEnded) { + return requiresConfirmation; + } + hasDynamicOperand = true; + } + if (!optionsEnded && argument === '--') { + optionsEnded = true; + continue; } - if (word.value.startsWith('--')) { - const optionName = word.value.split('=', 1)[0]; - if (optionName.length >= 3 && inPlaceLongOption.startsWith(optionName)) { - return SedCommandAnalysis.RequiresConfirmation; + if (!optionsEnded && argument.startsWith('--')) { + const optionName = argument.split('=', 1)[0]; + if (style === 'gnu' && optionName.length >= 3 && inPlaceLongOption.startsWith(optionName)) { + if (inPlaceSuffix !== undefined) { + return requiresConfirmation; + } + inPlaceSuffix = argument.includes('=') ? argument.slice(argument.indexOf('=') + 1) : ''; + continue; + } + if (isLongOptionAbbreviation(optionName, '--expression', 3) || isLongOptionAbbreviation(optionName, '--file', 4)) { + if (optionName !== '--expression' && optionName !== '--file') { + hasUnknownOption = true; + } + hasScriptOption = true; + if (!argument.includes('=')) { + if (++index >= arguments_.length) { + return requiresConfirmation; + } + if (arguments_[index].hasRuntimeExpansion) { + return requiresConfirmation; + } + } + continue; + } + if (!isKnownNoArgumentLongOption(optionName)) { + hasUnknownOption = true; } continue; } - if (word.value.startsWith('-') && /[iI]/.test(word.value.slice(1))) { - return SedCommandAnalysis.RequiresConfirmation; + if (!optionsEnded && argument.startsWith('-') && argument.length > 1) { + const shortOption = parseShortOptions(argument.slice(1), style); + if (shortOption.kind === 'requiresConfirmation') { + return shortOption; + } + if (shortOption.inPlaceSuffix !== undefined) { + if (inPlaceSuffix !== undefined) { + return requiresConfirmation; + } + inPlaceSuffix = shortOption.inPlaceSuffix; + if (shortOption.consumesNextAsSuffix) { + if (++index >= arguments_.length) { + return requiresConfirmation; + } + const suffixWord = arguments_[index]; + if (suffixWord.hasRuntimeExpansion) { + return requiresConfirmation; + } + inPlaceSuffix = suffixWord.value; + } + } + if (shortOption.hasScriptOption) { + hasScriptOption = true; + if (shortOption.consumesNextAsScript) { + if (++index >= arguments_.length || arguments_[index].hasRuntimeExpansion) { + return requiresConfirmation; + } + } + } + hasUnknownOption ||= shortOption.hasUnknownOption; + continue; } + operands.push(argument); + } + + if (inPlaceSuffix === undefined) { + return { kind: 'safe' }; + } + if (hasUnknownOption || hasDynamicOperand) { + return requiresConfirmation; + } + const fileTargets = hasScriptOption ? operands : operands.slice(1); + if (fileTargets.length === 0) { + return { kind: 'invalidInPlace' }; } - return SedCommandAnalysis.Safe; + const fileWrites = fileTargets.flatMap(target => getInPlaceFileWrites(target, inPlaceSuffix, style)); + return { kind: 'inPlace', fileWrites }; } -function tokenizeCommand(commandText: string): IShellWord[] { +function parseShortOptions(flags: string, style: 'gnu' | 'bsd'): { + readonly kind: 'parsed'; + readonly inPlaceSuffix?: string; + readonly consumesNextAsSuffix: boolean; + readonly hasScriptOption: boolean; + readonly consumesNextAsScript: boolean; + readonly hasUnknownOption: boolean; +} | { readonly kind: 'requiresConfirmation' } { + let hasUnknownOption = false; + for (let index = 0; index < flags.length; index++) { + const flag = flags[index]; + if (flag === 'e' || flag === 'f') { + return { + kind: 'parsed', + consumesNextAsSuffix: false, + hasScriptOption: true, + consumesNextAsScript: index === flags.length - 1, + hasUnknownOption, + }; + } + if (flag === 'i' || (style === 'bsd' && flag === 'I')) { + return { + kind: 'parsed', + inPlaceSuffix: flags.slice(index + 1), + consumesNextAsSuffix: style === 'bsd' && index === flags.length - 1, + hasScriptOption: false, + consumesNextAsScript: false, + hasUnknownOption, + }; + } + if (!'nErsuzl'.includes(flag)) { + hasUnknownOption = true; + } + } + return { + kind: 'parsed', + consumesNextAsSuffix: false, + hasScriptOption: false, + consumesNextAsScript: false, + hasUnknownOption, + }; +} + +function getInPlaceFileWrites(target: string, suffix: string, style: 'gnu' | 'bsd'): string[] { + if (!suffix) { + return [target]; + } + if (style === 'gnu' && suffix.includes('*')) { + return [target, suffix.replaceAll('*', target)]; + } + return [target, `${target}${suffix}`]; +} + +function isSedExecutable(value: string): boolean { + return /(?:^|[/\\])sed(?:\.exe)?$/.test(value) || /^sed\b/.test(value); +} + +function isLongOptionAbbreviation(optionName: string, fullName: string, minimumLength: number): boolean { + return optionName.length >= minimumLength && fullName.startsWith(optionName); +} + +function isKnownNoArgumentLongOption(optionName: string): boolean { + return [ + '--debug', + '--help', + '--null-data', + '--posix', + '--quiet', + '--regexp-extended', + '--sandbox', + '--separate', + '--silent', + '--unbuffered', + '--version', + ].includes(optionName); +} + +function tokenizeCommand(commandText: string, shellDialect: 'bash' | 'powershell'): IShellWord[] { const words: IShellWord[] = []; let value = ''; let hasRuntimeExpansion = false; @@ -61,7 +235,8 @@ function tokenizeCommand(commandText: string): IShellWord[] { wordStarted = false; }; - for (const char of commandText) { + for (let index = 0; index < commandText.length; index++) { + const char = commandText[index]; if (escaping) { wordStarted = true; if (char !== '\n') { @@ -70,10 +245,13 @@ function tokenizeCommand(commandText: string): IShellWord[] { escaping = false; continue; } - if (char === '\\' && quote !== '\'') { - wordStarted = true; - escaping = true; - continue; + if (char === '\\' && shellDialect === 'bash' && quote !== '\'') { + const next = commandText[index + 1]; + if (quote !== '"' || next === '$' || next === '`' || next === '"' || next === '\\' || next === '\n') { + wordStarted = true; + escaping = true; + continue; + } } if (char === quote) { quote = undefined; @@ -96,7 +274,6 @@ function tokenizeCommand(commandText: string): IShellWord[] { wordStarted = true; value += char; } - if (wordStarted) { if (quote || escaping) { hasRuntimeExpansion = true; diff --git a/src/vs/platform/terminal/test/common/sedCommandAnalyzer.test.ts b/src/vs/platform/terminal/test/common/sedCommandAnalyzer.test.ts index 79f7836d0f9da8..b82ead116ab069 100644 --- a/src/vs/platform/terminal/test/common/sedCommandAnalyzer.test.ts +++ b/src/vs/platform/terminal/test/common/sedCommandAnalyzer.test.ts @@ -5,7 +5,7 @@ import assert from 'assert'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js'; -import { analyzeSedCommand, SedCommandAnalysis } from '../../common/sedCommandAnalyzer.js'; +import { analyzeSedCommand } from '../../common/sedCommandAnalyzer.js'; suite('analyzeSedCommand', () => { @@ -24,14 +24,14 @@ suite('analyzeSedCommand', () => { assert.deepStrictEqual( commands.map(analyzeSedCommand), - commands.map(() => SedCommandAnalysis.Safe), + commands.map(() => ({ kind: 'safe' })), ); }); - test('requires confirmation for semantic in-place options', () => { + test('identifies static semantic in-place options', () => { const commands = [ 'sed -i "s/foo/bar/" file.txt', - 'sed -I "s/foo/bar/" file.txt', + 'sed -I .bak "s/foo/bar/" file.txt', 'sed -ni "s/foo/bar/" file.txt', 'sed -n -i "s/foo/bar/" file.txt', 'sed -i.bak "s/foo/bar/" file.txt', @@ -48,8 +48,24 @@ suite('analyzeSedCommand', () => { ]; assert.deepStrictEqual( - commands.map(analyzeSedCommand), - commands.map(() => SedCommandAnalysis.RequiresConfirmation), + commands.map(command => analyzeSedCommand(command).kind), + [ + 'inPlace', + 'inPlace', + 'inPlace', + 'inPlace', + 'inPlace', + 'inPlace', + 'inPlace', + 'inPlace', + 'inPlace', + 'inPlace', + 'inPlace', + 'inPlace', + 'requiresConfirmation', + 'inPlace', + 'requiresConfirmation', + ], ); }); @@ -66,7 +82,58 @@ suite('analyzeSedCommand', () => { assert.deepStrictEqual( commands.map(analyzeSedCommand), - commands.map(() => SedCommandAnalysis.RequiresConfirmation), + commands.map(() => ({ kind: 'requiresConfirmation' })), + ); + }); + + test('extracts the union of static GNU and BSD write destinations', () => { + const cases = [ + ['sed -i "s/foo/bar/" file.txt', ['file.txt']], + ['sed -i.bak "s/foo/bar/" file.txt', ['file.txt', 'file.txt.bak']], + ['sed --in-place "s/foo/bar/" file.txt', ['file.txt']], + ['sed --in-place=.bak "s/foo/bar/" file.txt', ['file.txt', 'file.txt.bak']], + ['sed -i "" "s/foo/bar/" file.txt', ['s/foo/bar/', 'file.txt']], + ['sed -i json "s/foo/bar/" package.', ['s/foo/bar/', 'package.', 'package.json']], + ['sed -I .json "s/foo/bar/" package', ['package', 'package.json']], + ['sed -i\'../outside/*\' "s/foo/bar/" inside.txt', ['inside.txt', '../outside/inside.txt', 'inside.txt../outside/*']], + ['sed -i "s/foo/bar/" file1.txt file2.txt', ['file1.txt', 'file2.txt', 'file2.txts/foo/bar/']], + ['sed --in-place -e "s/foo/bar/" file.txt', ['file.txt']], + ] as const; + + assert.deepStrictEqual( + cases.map(([commandLine]) => analyzeSedCommand(commandLine)), + cases.map(([, fileWrites]) => ({ kind: 'inPlace', fileWrites: [...fileWrites] })), + ); + }); + + test('decodes backslashes according to the shell dialect', () => { + assert.deepStrictEqual({ + bashUnquoted: analyzeSedCommand('sed --in-place "s/foo/bar/" \\/etc/config', 'bash'), + bashDoubleQuotedLiteral: analyzeSedCommand('sed --in-place "s/foo/bar/" "path\\q"', 'bash'), + bashDoubleQuotedEscapedExpansion: analyzeSedCommand('sed --in-place "s/foo/bar/" "path\\$FILE"', 'bash'), + powerShellPath: analyzeSedCommand('sed --in-place "s/foo/bar/" C:\\outside\\file.txt', 'powershell'), + }, { + bashUnquoted: { kind: 'inPlace', fileWrites: ['/etc/config'] }, + bashDoubleQuotedLiteral: { kind: 'inPlace', fileWrites: ['path\\q'] }, + bashDoubleQuotedEscapedExpansion: { kind: 'inPlace', fileWrites: ['path$FILE'] }, + powerShellPath: { kind: 'inPlace', fileWrites: ['C:\\outside\\file.txt'] }, + }); + }); + + test('requires confirmation when static destinations cannot be determined', () => { + const commands = [ + 'sed -i "s/foo/bar/"', + 'sed --follow-symlinks -i "s/foo/bar/" link.txt', + 'sed --in-place --expr="s/foo/bar/" outside.txt', + 'sed --in-place --fi=script.sed outside.txt', + 'sed -i -x "s/foo/bar/" file.txt', + 'sed -i.bak "-e" $ARGS inside.txt', + 'sed --in-place --file "$SCRIPT" inside.txt', + ]; + + assert.deepStrictEqual( + commands.map(analyzeSedCommand), + commands.map(() => ({ kind: 'requiresConfirmation' })), ); }); }); diff --git a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/commandParsers/commandFileWriteParser.ts b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/commandParsers/commandFileWriteParser.ts index 09d14d75e422d2..d521ac9e0b8e44 100644 --- a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/commandParsers/commandFileWriteParser.ts +++ b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/commandParsers/commandFileWriteParser.ts @@ -25,7 +25,8 @@ export interface ICommandFileWriteParser { * Extracts the file paths that would be written to by this command. * Should only be called if canHandle() returns true. * @param commandText The full text of a single command (not a pipeline). - * @returns Array of file paths that would be modified. + * @returns Array of file paths that would be modified. An undefined entry + * indicates a write whose destination cannot be determined statically. */ - extractFileWrites(commandText: string): string[]; + extractFileWrites(commandText: string): (string | undefined)[]; } diff --git a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/commandParsers/sedFileWriteParser.ts b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/commandParsers/sedFileWriteParser.ts index f1442781c74273..58e69b62f04f53 100644 --- a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/commandParsers/sedFileWriteParser.ts +++ b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/commandParsers/sedFileWriteParser.ts @@ -4,6 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import { ICommandFileWriteParser } from './commandFileWriteParser.js'; +import { analyzeSedCommand } from '../../../../../../platform/terminal/common/sedCommandAnalyzer.js'; /** * Parser for detecting file writes from `sed` commands using in-place editing. @@ -20,193 +21,14 @@ export class SedFileWriteParser implements ICommandFileWriteParser { readonly commandName = 'sed'; canHandle(commandText: string): boolean { - // Check if this is a sed command - if (!commandText.match(/^sed\s+/)) { - return false; - } - - // Check for -i, -I, or --in-place flag - const inPlaceRegex = /(?:^|\s)(-[a-zA-Z]*[iI][a-zA-Z]*\S*|--in-place(?:=\S*)?|(-i|-I)\s*'[^']*'|(-i|-I)\s*"[^"]*")(?:\s|$)/; - return inPlaceRegex.test(commandText); - } - - extractFileWrites(commandText: string): string[] { - const tokens = this._tokenizeCommand(commandText); - return this._extractFileTargets(tokens); - } - - /** - * Tokenizes a command into individual arguments, handling quotes and escapes. - */ - private _tokenizeCommand(commandText: string): string[] { - const tokens: string[] = []; - let current = ''; - let inSingleQuote = false; - let inDoubleQuote = false; - let escaped = false; - - for (let i = 0; i < commandText.length; i++) { - const char = commandText[i]; - - if (escaped) { - current += char; - escaped = false; - continue; - } - - if (char === '\\' && !inSingleQuote) { - escaped = true; - current += char; - continue; - } - - if (char === '\'' && !inDoubleQuote) { - inSingleQuote = !inSingleQuote; - current += char; - continue; - } - - if (char === '"' && !inSingleQuote) { - inDoubleQuote = !inDoubleQuote; - current += char; - continue; - } - - if (/\s/.test(char) && !inSingleQuote && !inDoubleQuote) { - if (current) { - tokens.push(current); - current = ''; - } - continue; - } - - current += char; - } - - if (current) { - tokens.push(current); - } - - return tokens; + return analyzeSedCommand(commandText, 'bash').kind !== 'safe'; } - /** - * Extracts file targets from tokenized sed command arguments. - * Files are generally the last non-option, non-script arguments. - */ - private _extractFileTargets(tokens: string[]): string[] { - if (tokens.length === 0 || tokens[0] !== 'sed') { - return []; - } - - const files: string[] = []; - let i = 1; // Skip 'sed' - let foundScript = false; - - while (i < tokens.length) { - const token = tokens[i]; - - // Long options - if (token.startsWith('--')) { - if (token === '--in-place' || token.startsWith('--in-place=')) { - // In-place flag (already verified we have one) - i++; - continue; - } - if (token === '--expression' || token === '--file') { - // Skip the option and its argument - i += 2; - foundScript = true; - continue; - } - if (token.startsWith('--expression=') || token.startsWith('--file=')) { - i++; - foundScript = true; - continue; - } - // Other long options like --sandbox, --debug, etc. - i++; - continue; - } - - // Short options - if (token.startsWith('-') && token.length > 1 && token[1] !== '-') { - // Could be combined flags like -ni or -i.bak - const flags = token.slice(1); - - // Check if this is -i with backup suffix attached (e.g., -i.bak) - const iIndex = flags.indexOf('i'); - const IIndex = flags.indexOf('I'); - const inPlaceIndex = iIndex >= 0 ? iIndex : IIndex; - - if (inPlaceIndex >= 0 && inPlaceIndex < flags.length - 1) { - // -i.bak style - backup suffix is attached - i++; - continue; - } - - // Check if -i or -I is the last flag and next token could be backup suffix - if ((flags.endsWith('i') || flags.endsWith('I')) && i + 1 < tokens.length) { - const nextToken = tokens[i + 1]; - // macOS/BSD style: -i '' or -i "" (empty string backup suffix) - // Only treat it as a backup suffix if it's empty or looks like a backup - // extension (starts with '.' and is short). Don't match sed scripts like 's/foo/bar/'. - if (nextToken === '\'\'' || nextToken === '""') { - i += 2; - continue; - } - // Check for quoted backup suffixes like '.bak' or ".backup" - if ((nextToken.startsWith('\'') && nextToken.endsWith('\'')) || (nextToken.startsWith('"') && nextToken.endsWith('"'))) { - const unquoted = nextToken.slice(1, -1); - // Backup suffixes typically start with '.' and are short extensions - if (unquoted.startsWith('.') && unquoted.length <= 10 && !unquoted.includes('/')) { - i += 2; - continue; - } - } - } - - // Check for -e or -f which take arguments - if (flags.includes('e') || flags.includes('f')) { - const eIndex = flags.indexOf('e'); - const fIndex = flags.indexOf('f'); - const optIndex = eIndex >= 0 ? eIndex : fIndex; - - // If -e or -f is not the last character, the rest of the token is the argument - if (optIndex < flags.length - 1) { - foundScript = true; - i++; - continue; - } - - // Otherwise, the next token is the argument - foundScript = true; - i += 2; - continue; - } - - i++; - continue; - } - - // Non-option argument - if (!foundScript) { - // First non-option is the script (unless -e/-f was used) - foundScript = true; - i++; - continue; - } - - // Subsequent non-option arguments are files - // Strip surrounding quotes from file path - let file = token; - if ((file.startsWith('\'') && file.endsWith('\'')) || (file.startsWith('"') && file.endsWith('"'))) { - file = file.slice(1, -1); - } - files.push(file); - i++; + extractFileWrites(commandText: string): (string | undefined)[] { + const analysis = analyzeSedCommand(commandText, 'bash'); + if (analysis.kind === 'requiresConfirmation') { + return [undefined]; } - - return files; + return analysis.kind === 'inPlace' ? [...analysis.fileWrites] : []; } } diff --git a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/commandLineAnalyzer/commandLineFileWriteAnalyzer.ts b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/commandLineAnalyzer/commandLineFileWriteAnalyzer.ts index c30f6a83009592..08810e94f7431b 100644 --- a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/commandLineAnalyzer/commandLineFileWriteAnalyzer.ts +++ b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/commandLineAnalyzer/commandLineFileWriteAnalyzer.ts @@ -19,7 +19,7 @@ import { ILabelService } from '../../../../../../../platform/label/common/label. const nullDevice = Symbol('null device'); -type FileWrite = URI | string | typeof nullDevice; +type FileWrite = URI | string | typeof nullDevice | undefined; export class CommandLineFileWriteAnalyzer extends Disposable implements ICommandLineAnalyzer { constructor( @@ -64,7 +64,7 @@ export class CommandLineFileWriteAnalyzer extends Disposable implements ICommand if (cwd) { this._log('Detected cwd', cwd.toString()); fileWrites = allCapturedFileWrites.map(e => { - if (e === nullDevice) { + if (e === nullDevice || e === undefined) { return e; } @@ -92,7 +92,7 @@ export class CommandLineFileWriteAnalyzer extends Disposable implements ICommand fileWrites = allCapturedFileWrites; } } - this._log('File writes detected', fileWrites.map(e => e.toString())); + this._log('File writes detected', fileWrites.map(e => e?.toString() ?? 'unknown')); return fileWrites; } @@ -107,7 +107,10 @@ export class CommandLineFileWriteAnalyzer extends Disposable implements ICommand return result; } - private _mapNullDevice(options: ICommandLineAnalyzerOptions, rawFileWrite: string): string | typeof nullDevice { + private _mapNullDevice(options: ICommandLineAnalyzerOptions, rawFileWrite: string | undefined): string | typeof nullDevice | undefined { + if (rawFileWrite === undefined) { + return undefined; + } if (options.treeSitterLanguage === TreeSitterCommandParserLanguage.PowerShell) { return rawFileWrite === '$null' ? nullDevice @@ -132,6 +135,11 @@ export class CommandLineFileWriteAnalyzer extends Disposable implements ICommand const workspaceFolders = this._workspaceContextService.getWorkspace().folders; if (workspaceFolders.length > 0) { for (const fileWrite of fileWrites) { + if (fileWrite === undefined) { + isAutoApproveAllowed = false; + this._log('File write blocked due to unknown destination'); + break; + } if (fileWrite === nullDevice) { this._log('File write to null device allowed', URI.isUri(fileWrite) ? fileWrite.toString() : fileWrite); continue; @@ -193,7 +201,7 @@ export class CommandLineFileWriteAnalyzer extends Disposable implements ICommand const disclaimers: string[] = []; if (fileWrites.length > 0) { - const fileWritesList = fileWrites.map(fw => `\`${URI.isUri(fw) ? this._labelService.getUriLabel(fw) : fw === nullDevice ? '/dev/null' : fw.toString()}\``).join(', '); + const fileWritesList = fileWrites.map(fw => `\`${URI.isUri(fw) ? this._labelService.getUriLabel(fw) : fw === nullDevice ? '/dev/null' : fw?.toString() ?? localize('unknownFileWriteDestination', "unknown destination")}\``).join(', '); if (!isAutoApproveAllowed) { disclaimers.push(localize('runInTerminal.fileWriteBlockedDisclaimer', 'File write operations detected that cannot be auto approved: {0}', fileWritesList)); } else { diff --git a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/treeSitterCommandParser.ts b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/treeSitterCommandParser.ts index 5c2c7a5adccffb..e50fde8a43ba00 100644 --- a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/treeSitterCommandParser.ts +++ b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/treeSitterCommandParser.ts @@ -151,7 +151,7 @@ export class TreeSitterCommandParser extends Disposable { * Uses registered command parsers (e.g., for `sed -i`) to detect command-specific file writes. * Returns an array of file paths that would be modified. */ - async getCommandFileWrites(languageId: TreeSitterCommandParserLanguage, commandLine: string): Promise { + async getCommandFileWrites(languageId: TreeSitterCommandParserLanguage, commandLine: string): Promise<(string | undefined)[]> { // Currently only bash-like shells are supported for command-specific parsing if (languageId !== TreeSitterCommandParserLanguage.Bash) { return []; @@ -161,7 +161,7 @@ export class TreeSitterCommandParser extends Disposable { const query = '(command) @command'; const captures = await this._queryTree(languageId, commandLine, query); - const result: string[] = []; + const result: (string | undefined)[] = []; for (const capture of captures) { const commandText = capture.node.text; for (const parser of this._commandFileWriteParsers) { diff --git a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/electron-browser/commandLineAnalyzer/commandLineFileWriteAnalyzer.test.ts b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/electron-browser/commandLineAnalyzer/commandLineFileWriteAnalyzer.test.ts index 4c881a55cb27d0..fb930036370c79 100644 --- a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/electron-browser/commandLineAnalyzer/commandLineFileWriteAnalyzer.test.ts +++ b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/electron-browser/commandLineAnalyzer/commandLineFileWriteAnalyzer.test.ts @@ -196,7 +196,7 @@ suite('CommandLineFileWriteAnalyzer', () => { suite('sed in-place editing', () => { // Basic -i flag variants (inside workspace) test('sed -i inside workspace - allow', () => t('sed -i \'s/foo/bar/\' file.txt', 'outsideWorkspace', true, 1)); - test('sed -I (uppercase) inside workspace - allow', () => t('sed -I \'s/foo/bar/\' file.txt', 'outsideWorkspace', true, 1)); + test('sed -I (uppercase) inside workspace - allow', () => t('sed -I \'\' \'s/foo/bar/\' file.txt', 'outsideWorkspace', true, 1)); test('sed --in-place inside workspace - allow', () => t('sed --in-place \'s/foo/bar/\' file.txt', 'outsideWorkspace', true, 1)); // Backup suffix variants (inside workspace) @@ -222,6 +222,14 @@ suite('CommandLineFileWriteAnalyzer', () => { // With blockDetectedFileWrites: never test('sed -i with never setting - allow', () => t('sed -i \'s/foo/bar/\' file.txt', 'never', true, 1)); + // Shared sed analysis fails closed when destinations are ambiguous + test('sed -i missing target - block', () => t('sed -i \'s/foo/bar/\'', 'outsideWorkspace', false, 1)); + test('sed -i glob target - block', () => t('sed -i \'s/foo/bar/\' *.txt', 'outsideWorkspace', false, 1)); + test('sed runtime option - block', () => t('sed "$SED_OPTIONS" \'s/foo/bar/\' file.txt', 'outsideWorkspace', false, 1)); + test('sed runtime expression - block', () => t('sed -i.bak -e "$SCRIPT" file.txt', 'outsideWorkspace', false, 1)); + test('sed --follow-symlinks -i - block', () => t('sed --follow-symlinks -i \'s/foo/bar/\' file.txt', 'outsideWorkspace', false, 1)); + test('sed backup suffix outside workspace - block', () => t('sed -i\'../outside/*\' \'s/foo/bar/\' file.txt', 'outsideWorkspace', false, 1)); + // Without -i flag (should not detect as file write) test('sed without -i - no file write detected', () => t('sed \'s/foo/bar/\' file.txt', 'outsideWorkspace', true, 0)); test('sed with pipe - no file write detected', () => t('cat file.txt | sed \'s/foo/bar/\'', 'outsideWorkspace', true, 0)); From fab0086ee9812d3e07ba18f9e809a2dc529cce1c Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Mon, 3 Aug 2026 20:03:33 -0700 Subject: [PATCH 3/6] Handle sed executable casing and invalid options Normalize sed executable matching for PowerShell and avoid treating option characters after an invalid short option as in-place flags. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bda42f98-e73f-417f-b7e6-03cff9e2f604 --- .../test/node/commandAutoApprover.test.ts | 11 +++++++++ .../terminal/common/sedCommandAnalyzer.ts | 23 +++++++++++-------- .../test/common/sedCommandAnalyzer.test.ts | 7 +++++- 3 files changed, 30 insertions(+), 11 deletions(-) diff --git a/src/vs/platform/agentHost/test/node/commandAutoApprover.test.ts b/src/vs/platform/agentHost/test/node/commandAutoApprover.test.ts index 0e618b79ab14f1..0809b812eaaa08 100644 --- a/src/vs/platform/agentHost/test/node/commandAutoApprover.test.ts +++ b/src/vs/platform/agentHost/test/node/commandAutoApprover.test.ts @@ -154,6 +154,17 @@ suite('CommandAutoApprover', () => { rejected: { result: 'noMatch', autoApproveRuleResolvable: false }, accepted: { result: 'approved', autoApproveRuleResolvable: false }, }); + + test('checks PowerShell sed executable casing', () => { + const options = { + language: 'powershell' as const, + isWriteDestApproved: (dest: string) => dest === 'file.txt', + }; + assert.deepStrictEqual([ + approver.shouldAutoApprove('SED -i "s/foo/bar/" file.txt', options), + approver.shouldAutoApprove('SED.EXE -i "s/foo/bar/" file.txt', options), + ], ['approved', 'approved']); + }); }); // npm/package managers diff --git a/src/vs/platform/terminal/common/sedCommandAnalyzer.ts b/src/vs/platform/terminal/common/sedCommandAnalyzer.ts index 7fa1a1dde14611..cb39bef051f1bd 100644 --- a/src/vs/platform/terminal/common/sedCommandAnalyzer.ts +++ b/src/vs/platform/terminal/common/sedCommandAnalyzer.ts @@ -14,7 +14,7 @@ interface IShellWord { } interface ISedParseResult { - readonly kind: 'safe' | 'inPlace' | 'invalidInPlace' | 'requiresConfirmation'; + readonly kind: 'safe' | 'inPlace' | 'invalid' | 'invalidInPlace' | 'requiresConfirmation'; readonly fileWrites?: readonly string[]; } @@ -25,7 +25,7 @@ const inPlaceLongOption = '--in-place'; export function analyzeSedCommand(commandText: string, shellDialect: 'bash' | 'powershell' = 'bash'): SedCommandAnalysis { const words = tokenizeCommand(commandText, shellDialect); const executable = words[0]; - if (!executable || !isSedExecutable(executable.value)) { + if (!executable || !isSedExecutable(executable.value, shellDialect)) { return safe; } if (executable.hasRuntimeExpansion) { @@ -99,6 +99,9 @@ function parseSedArguments(arguments_: readonly IShellWord[], style: 'gnu' | 'bs if (shortOption.kind === 'requiresConfirmation') { return shortOption; } + if (shortOption.kind === 'invalid') { + return shortOption; + } if (shortOption.inPlaceSuffix !== undefined) { if (inPlaceSuffix !== undefined) { return requiresConfirmation; @@ -150,8 +153,7 @@ function parseShortOptions(flags: string, style: 'gnu' | 'bsd'): { readonly hasScriptOption: boolean; readonly consumesNextAsScript: boolean; readonly hasUnknownOption: boolean; -} | { readonly kind: 'requiresConfirmation' } { - let hasUnknownOption = false; +} | { readonly kind: 'invalid' } | { readonly kind: 'requiresConfirmation' } { for (let index = 0; index < flags.length; index++) { const flag = flags[index]; if (flag === 'e' || flag === 'f') { @@ -160,7 +162,7 @@ function parseShortOptions(flags: string, style: 'gnu' | 'bsd'): { consumesNextAsSuffix: false, hasScriptOption: true, consumesNextAsScript: index === flags.length - 1, - hasUnknownOption, + hasUnknownOption: false, }; } if (flag === 'i' || (style === 'bsd' && flag === 'I')) { @@ -170,11 +172,11 @@ function parseShortOptions(flags: string, style: 'gnu' | 'bsd'): { consumesNextAsSuffix: style === 'bsd' && index === flags.length - 1, hasScriptOption: false, consumesNextAsScript: false, - hasUnknownOption, + hasUnknownOption: false, }; } if (!'nErsuzl'.includes(flag)) { - hasUnknownOption = true; + return { kind: 'invalid' }; } } return { @@ -182,7 +184,7 @@ function parseShortOptions(flags: string, style: 'gnu' | 'bsd'): { consumesNextAsSuffix: false, hasScriptOption: false, consumesNextAsScript: false, - hasUnknownOption, + hasUnknownOption: false, }; } @@ -196,8 +198,9 @@ function getInPlaceFileWrites(target: string, suffix: string, style: 'gnu' | 'bs return [target, `${target}${suffix}`]; } -function isSedExecutable(value: string): boolean { - return /(?:^|[/\\])sed(?:\.exe)?$/.test(value) || /^sed\b/.test(value); +function isSedExecutable(value: string, shellDialect: 'bash' | 'powershell'): boolean { + const normalized = shellDialect === 'powershell' ? value.toLowerCase() : value; + return /(?:^|[/\\])sed(?:\.exe)?$/.test(normalized) || /^sed\b/.test(normalized); } function isLongOptionAbbreviation(optionName: string, fullName: string, minimumLength: number): boolean { diff --git a/src/vs/platform/terminal/test/common/sedCommandAnalyzer.test.ts b/src/vs/platform/terminal/test/common/sedCommandAnalyzer.test.ts index b82ead116ab069..e9745b686440f2 100644 --- a/src/vs/platform/terminal/test/common/sedCommandAnalyzer.test.ts +++ b/src/vs/platform/terminal/test/common/sedCommandAnalyzer.test.ts @@ -20,6 +20,7 @@ suite('analyzeSedCommand', () => { 'sed --quiet "s/foo/bar/p" file.txt', 'sed --sandbox "s/foo/bar/" file.txt', 'sed -- "$SED_OPTIONS" file.txt', + 'sed "s/foo/bar/" "-\\inside.txt"', ]; assert.deepStrictEqual( @@ -98,6 +99,7 @@ suite('analyzeSedCommand', () => { ['sed -i\'../outside/*\' "s/foo/bar/" inside.txt', ['inside.txt', '../outside/inside.txt', 'inside.txt../outside/*']], ['sed -i "s/foo/bar/" file1.txt file2.txt', ['file1.txt', 'file2.txt', 'file2.txts/foo/bar/']], ['sed --in-place -e "s/foo/bar/" file.txt', ['file.txt']], + ['sed -i -x "s/foo/bar/" file.txt', ['file.txt', 'file.txt-x']], ] as const; assert.deepStrictEqual( @@ -112,11 +114,15 @@ suite('analyzeSedCommand', () => { bashDoubleQuotedLiteral: analyzeSedCommand('sed --in-place "s/foo/bar/" "path\\q"', 'bash'), bashDoubleQuotedEscapedExpansion: analyzeSedCommand('sed --in-place "s/foo/bar/" "path\\$FILE"', 'bash'), powerShellPath: analyzeSedCommand('sed --in-place "s/foo/bar/" C:\\outside\\file.txt', 'powershell'), + powerShellUppercase: analyzeSedCommand('SED -i "s/foo/bar/" file.txt', 'powershell'), + powerShellUppercaseExe: analyzeSedCommand('SED.EXE -i "s/foo/bar/" file.txt', 'powershell'), }, { bashUnquoted: { kind: 'inPlace', fileWrites: ['/etc/config'] }, bashDoubleQuotedLiteral: { kind: 'inPlace', fileWrites: ['path\\q'] }, bashDoubleQuotedEscapedExpansion: { kind: 'inPlace', fileWrites: ['path$FILE'] }, powerShellPath: { kind: 'inPlace', fileWrites: ['C:\\outside\\file.txt'] }, + powerShellUppercase: { kind: 'inPlace', fileWrites: ['file.txt'] }, + powerShellUppercaseExe: { kind: 'inPlace', fileWrites: ['file.txt'] }, }); }); @@ -126,7 +132,6 @@ suite('analyzeSedCommand', () => { 'sed --follow-symlinks -i "s/foo/bar/" link.txt', 'sed --in-place --expr="s/foo/bar/" outside.txt', 'sed --in-place --fi=script.sed outside.txt', - 'sed -i -x "s/foo/bar/" file.txt', 'sed -i.bak "-e" $ARGS inside.txt', 'sed --in-place --file "$SCRIPT" inside.txt', ]; From 5baca077c4f38100ef2977808fe2783927954f9c Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Mon, 3 Aug 2026 22:39:39 -0700 Subject: [PATCH 4/6] Fix sed analyzer CI validation Use explicit analyzer callbacks for TypeScript and quote native Windows paths in Bash session-policy tests so shell decoding preserves the absolute path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bda42f98-e73f-417f-b7e6-03cff9e2f604 --- .../agentHost/test/node/sessionPermissions.test.ts | 2 +- .../terminal/test/common/sedCommandAnalyzer.test.ts | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/vs/platform/agentHost/test/node/sessionPermissions.test.ts b/src/vs/platform/agentHost/test/node/sessionPermissions.test.ts index 2a4a1d48a6b06e..23b6373965d57b 100644 --- a/src/vs/platform/agentHost/test/node/sessionPermissions.test.ts +++ b/src/vs/platform/agentHost/test/node/sessionPermissions.test.ts @@ -243,7 +243,7 @@ suite('SessionPermissionManager', () => { const cases: [commandLine: string, expected: ToolCallConfirmationReason | undefined][] = [ ['sed -i "s/foo/bar/" file.txt', ToolCallConfirmationReason.NotNeeded], ['sed -i.bak "s/foo/bar/" file.txt', ToolCallConfirmationReason.NotNeeded], - [`sed --in-place "s/foo/bar/" ${join(outsideDir, 'file.txt')}`, undefined], + [`sed --in-place "s/foo/bar/" "${join(outsideDir, 'file.txt')}"`, undefined], ['sed -i "s/foo/bar/" package.json', undefined], ['sed "$SED_OPTIONS" "s/foo/bar/" file.txt', undefined], ]; diff --git a/src/vs/platform/terminal/test/common/sedCommandAnalyzer.test.ts b/src/vs/platform/terminal/test/common/sedCommandAnalyzer.test.ts index e9745b686440f2..aa0097a88983c6 100644 --- a/src/vs/platform/terminal/test/common/sedCommandAnalyzer.test.ts +++ b/src/vs/platform/terminal/test/common/sedCommandAnalyzer.test.ts @@ -24,7 +24,7 @@ suite('analyzeSedCommand', () => { ]; assert.deepStrictEqual( - commands.map(analyzeSedCommand), + commands.map(command => analyzeSedCommand(command)), commands.map(() => ({ kind: 'safe' })), ); }); @@ -82,7 +82,7 @@ suite('analyzeSedCommand', () => { ]; assert.deepStrictEqual( - commands.map(analyzeSedCommand), + commands.map(command => analyzeSedCommand(command)), commands.map(() => ({ kind: 'requiresConfirmation' })), ); }); @@ -113,6 +113,7 @@ suite('analyzeSedCommand', () => { bashUnquoted: analyzeSedCommand('sed --in-place "s/foo/bar/" \\/etc/config', 'bash'), bashDoubleQuotedLiteral: analyzeSedCommand('sed --in-place "s/foo/bar/" "path\\q"', 'bash'), bashDoubleQuotedEscapedExpansion: analyzeSedCommand('sed --in-place "s/foo/bar/" "path\\$FILE"', 'bash'), + bashDoubleQuotedWindowsPath: analyzeSedCommand('sed --in-place "s/foo/bar/" "C:\\outside\\file.txt"', 'bash'), powerShellPath: analyzeSedCommand('sed --in-place "s/foo/bar/" C:\\outside\\file.txt', 'powershell'), powerShellUppercase: analyzeSedCommand('SED -i "s/foo/bar/" file.txt', 'powershell'), powerShellUppercaseExe: analyzeSedCommand('SED.EXE -i "s/foo/bar/" file.txt', 'powershell'), @@ -120,6 +121,7 @@ suite('analyzeSedCommand', () => { bashUnquoted: { kind: 'inPlace', fileWrites: ['/etc/config'] }, bashDoubleQuotedLiteral: { kind: 'inPlace', fileWrites: ['path\\q'] }, bashDoubleQuotedEscapedExpansion: { kind: 'inPlace', fileWrites: ['path$FILE'] }, + bashDoubleQuotedWindowsPath: { kind: 'inPlace', fileWrites: ['C:\\outside\\file.txt'] }, powerShellPath: { kind: 'inPlace', fileWrites: ['C:\\outside\\file.txt'] }, powerShellUppercase: { kind: 'inPlace', fileWrites: ['file.txt'] }, powerShellUppercaseExe: { kind: 'inPlace', fileWrites: ['file.txt'] }, @@ -137,7 +139,7 @@ suite('analyzeSedCommand', () => { ]; assert.deepStrictEqual( - commands.map(analyzeSedCommand), + commands.map(command => analyzeSedCommand(command)), commands.map(() => ({ kind: 'requiresConfirmation' })), ); }); From f9c707f217ad4024843f2771b88f6f71ef62f7c9 Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Mon, 3 Aug 2026 23:17:48 -0700 Subject: [PATCH 5/6] Scope sed in-place auto-approval fix Move the existing workbench sed parser to terminal common and reuse its in-place detection as a non-overridable Agent Host confirmation gate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bda42f98-e73f-417f-b7e6-03cff9e2f604 --- .../agentHost/node/commandAutoApprover.ts | 20 +- .../test/node/commandAutoApprover.test.ts | 109 ++----- .../test/node/sessionPermissions.test.ts | 25 +- .../terminal/common/sedCommandAnalyzer.ts | 287 ------------------ .../terminal/common/sedFileWriteParser.ts | 210 +++++++++++++ .../test/common/sedCommandAnalyzer.test.ts | 146 --------- .../test/common/sedFileWriteParser.test.ts | 49 +++ .../commandParsers/commandFileWriteParser.ts | 5 +- .../commandParsers/sedFileWriteParser.ts | 34 --- .../commandLineFileWriteAnalyzer.ts | 18 +- .../browser/treeSitterCommandParser.ts | 6 +- .../commandLineFileWriteAnalyzer.test.ts | 10 +- 12 files changed, 314 insertions(+), 605 deletions(-) delete mode 100644 src/vs/platform/terminal/common/sedCommandAnalyzer.ts create mode 100644 src/vs/platform/terminal/common/sedFileWriteParser.ts delete mode 100644 src/vs/platform/terminal/test/common/sedCommandAnalyzer.test.ts create mode 100644 src/vs/platform/terminal/test/common/sedFileWriteParser.test.ts delete mode 100644 src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/commandParsers/sedFileWriteParser.ts diff --git a/src/vs/platform/agentHost/node/commandAutoApprover.ts b/src/vs/platform/agentHost/node/commandAutoApprover.ts index bda8e77ebfdce0..2586ddb6703fe8 100644 --- a/src/vs/platform/agentHost/node/commandAutoApprover.ts +++ b/src/vs/platform/agentHost/node/commandAutoApprover.ts @@ -11,7 +11,7 @@ import { escapeRegExpCharacters, regExpLeadsToEndlessLoop } from '../../../base/ import { URI } from '../../../base/common/uri.js'; import { getAppNodeModulesPath } from './appNodeModules.js'; import { ILogService } from '../../log/common/log.js'; -import { analyzeSedCommand } from '../../terminal/common/sedCommandAnalyzer.js'; +import { SedFileWriteParser } from '../../terminal/common/sedFileWriteParser.js'; import type { AgentHostTerminalAutoApproveRuleValue, AgentHostTerminalAutoApproveRules } from '../common/agentHostSchema.js'; /** @@ -171,6 +171,7 @@ interface IAutoApproveRules { const neverMatchRegex = /(?!.*)/; const transientEnvVarRegex = /^[A-Z_][A-Z0-9_]*=/i; +const sedFileWriteParser = new SedFileWriteParser(); /** * Auto-approves or denies shell commands based on terminal auto-approve rules. @@ -246,28 +247,25 @@ export class CommandAutoApprover extends Disposable { return { result: 'noMatch', autoApproveRuleResolvable: false }; } - const sedAnalyses = parsed.subCommands.map(subCommand => analyzeSedCommand(subCommand, isPowerShell ? 'powershell' : 'bash')); - if (sedAnalyses.some(analysis => analysis.kind === 'requiresConfirmation')) { - return { result: 'denied', autoApproveRuleResolvable: false }; - } - const sedWriteDests = sedAnalyses.flatMap(analysis => analysis.kind === 'inPlace' ? analysis.fileWrites : []); - const writeDests = [...parsed.unsafeWriteDests, ...sedWriteDests]; - const hasUnapprovedWriteDest = () => writeDests.some(dest => dest === undefined || !options?.isWriteDestApproved?.(dest)); + const hasUnapprovedRedirect = () => parsed.unsafeWriteDests.some(dest => dest === undefined || !options?.isWriteDestApproved?.(dest)); let result = this._matchSubCommands(parsed.subCommands, rules, isPowerShell); if (result !== 'denied' && this._matchesCommandLineRule(trimmed, rules.allowCommandLineRules)) { result = 'approved'; } - if (result === 'approved' && hasUnapprovedWriteDest()) { - this._logService.trace('[CommandAutoApprover] Write to non-approved destination, requiring confirmation'); + if (result === 'approved' && hasUnapprovedRedirect()) { + this._logService.trace('[CommandAutoApprover] Write redirection to non-approved destination, requiring confirmation'); return { result: 'noMatch', autoApproveRuleResolvable: false }; } - return { result, autoApproveRuleResolvable: result === 'noMatch' && !hasUnapprovedWriteDest() }; + return { result, autoApproveRuleResolvable: result === 'noMatch' && !hasUnapprovedRedirect() }; } private _matchSubCommands(subCommands: string[], rules: IAutoApproveRules, isPowerShell: boolean): CommandApprovalResult { let allApproved = true; for (const subCommand of subCommands) { + if (sedFileWriteParser.canHandle(subCommand)) { + return 'denied'; + } // Deny transient env var assignments if (transientEnvVarRegex.test(subCommand)) { return 'denied'; diff --git a/src/vs/platform/agentHost/test/node/commandAutoApprover.test.ts b/src/vs/platform/agentHost/test/node/commandAutoApprover.test.ts index 0809b812eaaa08..ba1dfaadd0d2b3 100644 --- a/src/vs/platform/agentHost/test/node/commandAutoApprover.test.ts +++ b/src/vs/platform/agentHost/test/node/commandAutoApprover.test.ts @@ -81,90 +81,39 @@ suite('CommandAutoApprover', () => { }); test('handles sed with blocked args', () => { - assert.strictEqual(approver.shouldAutoApprove('sed "s/foo/bar/g" file.txt'), 'approved'); - assert.strictEqual(approver.shouldAutoApprove('sed -e "s/foo/bar/"'), 'denied'); - assert.strictEqual(approver.shouldAutoApprove('sed --expression "s/foo/bar/"'), 'denied'); - }); - - test('checks static sed in-place write destinations', () => { - const seen: string[] = []; - const options = { - isWriteDestApproved: (dest: string) => { - seen.push(dest); - return dest === 'file.txt' || dest === 'file.txt.bak'; - }, - }; - const commands = [ - 'sed -i "s/foo/bar/" file.txt', - 'sed -I .bak "s/foo/bar/" file.txt', - 'sed -ni "s/foo/bar/" file.txt', - 'sed -i.bak "s/foo/bar/" file.txt', - 'sed --in-place "s/foo/bar/" file.txt', - 'sed --in-plac "s/foo/bar/" file.txt', - 'sed.exe -i "s/foo/bar/" file.txt', - 'sed -\\i "s/foo/bar/" file.txt', - ]; - assert.deepStrictEqual( - commands.map(commandLine => approver.shouldAutoApprove(commandLine, options)), - commands.map(() => 'approved'), - ); - assert.deepStrictEqual(seen, [ - 'file.txt', - 'file.txt', - 'file.txt.bak', - 'file.txt', - 'file.txt', - 'file.txt.bak', - 'file.txt', - 'file.txt', - 'file.txt', - 'file.txt', + assert.deepStrictEqual([ + approver.shouldAutoApprove('sed "s/foo/bar/g" file.txt'), + approver.shouldAutoApprove('sed -e "s/foo/bar/"'), + approver.shouldAutoApprove('sed --expression "s/foo/bar/"'), + approver.shouldAutoApprove('sed -i "s/foo/bar/" file.txt'), + approver.shouldAutoApprove('sed -I "s/foo/bar/" file.txt'), + approver.shouldAutoApprove('sed -ni "s/foo/bar/" file.txt'), + approver.shouldAutoApprove('sed -i.bak "s/foo/bar/" file.txt'), + approver.shouldAutoApprove('sed -i \'\' "s/foo/bar/" file.txt'), + approver.shouldAutoApprove('sed --in-place "s/foo/bar/" file.txt'), + approver.shouldAutoApprove('sed --in-place=.bak "s/foo/bar/" file.txt'), + ], [ + 'approved', + 'denied', + 'denied', + 'denied', + 'denied', + 'denied', + 'denied', + 'denied', + 'denied', + 'denied', ]); }); - test('requires confirmation for dynamic or ambiguous sed forms', () => { - const commands = [ - 'sed "$SED_OPTIONS" "s/foo/bar/" file.txt', - 'sed "s/foo/bar/" "$(echo --in-place)" file.txt', - 'sed${PATH:+} -i "s/foo/bar/" file.txt', - 'sed${PATH:+} "s/foo/bar/" file.txt', - 'sed --follow-symlinks -i "s/foo/bar/" file.txt', - 'sed --in-place --expr="s/foo/bar/" file.txt', - 'sed -i.bak "-e" $ARGS inside.txt', - 'sed --in-place --file "$SCRIPT" inside.txt', - ]; - assert.deepStrictEqual( - commands.map(commandLine => approver.shouldAutoApprove(commandLine)), - commands.map(() => 'denied'), - ); - }); - - test('sed write policy cannot be overridden by allow rules', () => { + test('sed in-place commands cannot be allowed by a full-command rule', () => { const commandLine = 'sed -i "s/foo/bar/" file.txt'; - const autoApproveRules = { - sed: true, - '/^sed -i "s\\/foo\\/bar\\/" file\\.txt$/': { approve: true, matchCommandLine: true }, - }; - assert.deepStrictEqual({ - withoutPredicate: approver.evaluate(commandLine, { autoApproveRules }), - rejected: approver.evaluate(commandLine, { autoApproveRules, isWriteDestApproved: () => false }), - accepted: approver.evaluate(commandLine, { autoApproveRules, isWriteDestApproved: () => true }), - }, { - withoutPredicate: { result: 'noMatch', autoApproveRuleResolvable: false }, - rejected: { result: 'noMatch', autoApproveRuleResolvable: false }, - accepted: { result: 'approved', autoApproveRuleResolvable: false }, - }); - - test('checks PowerShell sed executable casing', () => { - const options = { - language: 'powershell' as const, - isWriteDestApproved: (dest: string) => dest === 'file.txt', - }; - assert.deepStrictEqual([ - approver.shouldAutoApprove('SED -i "s/foo/bar/" file.txt', options), - approver.shouldAutoApprove('SED.EXE -i "s/foo/bar/" file.txt', options), - ], ['approved', 'approved']); - }); + assert.deepStrictEqual(approver.evaluate(commandLine, { + autoApproveRules: { + sed: true, + '/^sed -i "s\\/foo\\/bar\\/" file\\.txt$/': { approve: true, matchCommandLine: true }, + }, + }), { result: 'denied', autoApproveRuleResolvable: false }); }); // npm/package managers diff --git a/src/vs/platform/agentHost/test/node/sessionPermissions.test.ts b/src/vs/platform/agentHost/test/node/sessionPermissions.test.ts index 23b6373965d57b..1755d9c6d9d389 100644 --- a/src/vs/platform/agentHost/test/node/sessionPermissions.test.ts +++ b/src/vs/platform/agentHost/test/node/sessionPermissions.test.ts @@ -239,27 +239,14 @@ suite('SessionPermissionManager', () => { assert.strictEqual(result, ToolCallConfirmationReason.NotNeeded); }); - test('sed in-place edits use the shell destination policy', async () => { - const cases: [commandLine: string, expected: ToolCallConfirmationReason | undefined][] = [ - ['sed -i "s/foo/bar/" file.txt', ToolCallConfirmationReason.NotNeeded], - ['sed -i.bak "s/foo/bar/" file.txt', ToolCallConfirmationReason.NotNeeded], - [`sed --in-place "s/foo/bar/" "${join(outsideDir, 'file.txt')}"`, undefined], - ['sed -i "s/foo/bar/" package.json', undefined], - ['sed "$SED_OPTIONS" "s/foo/bar/" file.txt', undefined], - ]; - const approvals = []; - const ruleResolvable = []; - for (const [commandLine] of cases) { - const event = shellEvent(commandLine, 'bash'); - approvals.push(await permissions.getAutoApproval(event, sessionUri)); - ruleResolvable.push(permissions.isAutoApproveRuleResolvable(event, sessionUri)); - } + test('requires confirmation for sed in-place edits', async () => { + const event = shellEvent('sed -i "s/foo/bar/" file.txt', 'bash'); assert.deepStrictEqual({ - approvals, - ruleResolvable, + approval: await permissions.getAutoApproval(event, sessionUri), + ruleResolvable: permissions.isAutoApproveRuleResolvable(event, sessionUri), }, { - approvals: cases.map(([, expected]) => expected), - ruleResolvable: cases.map(() => false), + approval: undefined, + ruleResolvable: false, }); }); diff --git a/src/vs/platform/terminal/common/sedCommandAnalyzer.ts b/src/vs/platform/terminal/common/sedCommandAnalyzer.ts deleted file mode 100644 index cb39bef051f1bd..00000000000000 --- a/src/vs/platform/terminal/common/sedCommandAnalyzer.ts +++ /dev/null @@ -1,287 +0,0 @@ -/*--------------------------------------------------------------------------------------------- - * Copyright (c) Microsoft Corporation. All rights reserved. - * Licensed under the MIT License. See License.txt in the project root for license information. - *--------------------------------------------------------------------------------------------*/ - -export type SedCommandAnalysis = - | { readonly kind: 'safe' } - | { readonly kind: 'inPlace'; readonly fileWrites: readonly string[] } - | { readonly kind: 'requiresConfirmation' }; - -interface IShellWord { - readonly value: string; - readonly hasRuntimeExpansion: boolean; -} - -interface ISedParseResult { - readonly kind: 'safe' | 'inPlace' | 'invalid' | 'invalidInPlace' | 'requiresConfirmation'; - readonly fileWrites?: readonly string[]; -} - -const safe: SedCommandAnalysis = { kind: 'safe' }; -const requiresConfirmation: SedCommandAnalysis = { kind: 'requiresConfirmation' }; -const inPlaceLongOption = '--in-place'; - -export function analyzeSedCommand(commandText: string, shellDialect: 'bash' | 'powershell' = 'bash'): SedCommandAnalysis { - const words = tokenizeCommand(commandText, shellDialect); - const executable = words[0]; - if (!executable || !isSedExecutable(executable.value, shellDialect)) { - return safe; - } - if (executable.hasRuntimeExpansion) { - return requiresConfirmation; - } - - const results = [parseSedArguments(words.slice(1), 'gnu'), parseSedArguments(words.slice(1), 'bsd')]; - if (results.some(result => result.kind === 'requiresConfirmation')) { - return requiresConfirmation; - } - const inPlaceResults = results.filter((result): result is ISedParseResult & { kind: 'inPlace'; fileWrites: readonly string[] } => result.kind === 'inPlace'); - if (inPlaceResults.length === 0) { - return results.some(result => result.kind === 'invalidInPlace') ? requiresConfirmation : safe; - } - const fileWrites = [...new Set(inPlaceResults.flatMap(result => result.fileWrites))]; - return fileWrites.length > 0 ? { kind: 'inPlace', fileWrites } : requiresConfirmation; -} - -function parseSedArguments(arguments_: readonly IShellWord[], style: 'gnu' | 'bsd'): ISedParseResult { - const operands: string[] = []; - let inPlaceSuffix: string | undefined; - let hasScriptOption = false; - let hasUnknownOption = false; - let hasDynamicOperand = false; - let optionsEnded = false; - - for (let index = 0; index < arguments_.length; index++) { - const word = arguments_[index]; - const argument = word.value; - if (word.hasRuntimeExpansion) { - if (!optionsEnded) { - return requiresConfirmation; - } - hasDynamicOperand = true; - } - if (!optionsEnded && argument === '--') { - optionsEnded = true; - continue; - } - if (!optionsEnded && argument.startsWith('--')) { - const optionName = argument.split('=', 1)[0]; - if (style === 'gnu' && optionName.length >= 3 && inPlaceLongOption.startsWith(optionName)) { - if (inPlaceSuffix !== undefined) { - return requiresConfirmation; - } - inPlaceSuffix = argument.includes('=') ? argument.slice(argument.indexOf('=') + 1) : ''; - continue; - } - if (isLongOptionAbbreviation(optionName, '--expression', 3) || isLongOptionAbbreviation(optionName, '--file', 4)) { - if (optionName !== '--expression' && optionName !== '--file') { - hasUnknownOption = true; - } - hasScriptOption = true; - if (!argument.includes('=')) { - if (++index >= arguments_.length) { - return requiresConfirmation; - } - if (arguments_[index].hasRuntimeExpansion) { - return requiresConfirmation; - } - } - continue; - } - if (!isKnownNoArgumentLongOption(optionName)) { - hasUnknownOption = true; - } - continue; - } - if (!optionsEnded && argument.startsWith('-') && argument.length > 1) { - const shortOption = parseShortOptions(argument.slice(1), style); - if (shortOption.kind === 'requiresConfirmation') { - return shortOption; - } - if (shortOption.kind === 'invalid') { - return shortOption; - } - if (shortOption.inPlaceSuffix !== undefined) { - if (inPlaceSuffix !== undefined) { - return requiresConfirmation; - } - inPlaceSuffix = shortOption.inPlaceSuffix; - if (shortOption.consumesNextAsSuffix) { - if (++index >= arguments_.length) { - return requiresConfirmation; - } - const suffixWord = arguments_[index]; - if (suffixWord.hasRuntimeExpansion) { - return requiresConfirmation; - } - inPlaceSuffix = suffixWord.value; - } - } - if (shortOption.hasScriptOption) { - hasScriptOption = true; - if (shortOption.consumesNextAsScript) { - if (++index >= arguments_.length || arguments_[index].hasRuntimeExpansion) { - return requiresConfirmation; - } - } - } - hasUnknownOption ||= shortOption.hasUnknownOption; - continue; - } - operands.push(argument); - } - - if (inPlaceSuffix === undefined) { - return { kind: 'safe' }; - } - if (hasUnknownOption || hasDynamicOperand) { - return requiresConfirmation; - } - const fileTargets = hasScriptOption ? operands : operands.slice(1); - if (fileTargets.length === 0) { - return { kind: 'invalidInPlace' }; - } - const fileWrites = fileTargets.flatMap(target => getInPlaceFileWrites(target, inPlaceSuffix, style)); - return { kind: 'inPlace', fileWrites }; -} - -function parseShortOptions(flags: string, style: 'gnu' | 'bsd'): { - readonly kind: 'parsed'; - readonly inPlaceSuffix?: string; - readonly consumesNextAsSuffix: boolean; - readonly hasScriptOption: boolean; - readonly consumesNextAsScript: boolean; - readonly hasUnknownOption: boolean; -} | { readonly kind: 'invalid' } | { readonly kind: 'requiresConfirmation' } { - for (let index = 0; index < flags.length; index++) { - const flag = flags[index]; - if (flag === 'e' || flag === 'f') { - return { - kind: 'parsed', - consumesNextAsSuffix: false, - hasScriptOption: true, - consumesNextAsScript: index === flags.length - 1, - hasUnknownOption: false, - }; - } - if (flag === 'i' || (style === 'bsd' && flag === 'I')) { - return { - kind: 'parsed', - inPlaceSuffix: flags.slice(index + 1), - consumesNextAsSuffix: style === 'bsd' && index === flags.length - 1, - hasScriptOption: false, - consumesNextAsScript: false, - hasUnknownOption: false, - }; - } - if (!'nErsuzl'.includes(flag)) { - return { kind: 'invalid' }; - } - } - return { - kind: 'parsed', - consumesNextAsSuffix: false, - hasScriptOption: false, - consumesNextAsScript: false, - hasUnknownOption: false, - }; -} - -function getInPlaceFileWrites(target: string, suffix: string, style: 'gnu' | 'bsd'): string[] { - if (!suffix) { - return [target]; - } - if (style === 'gnu' && suffix.includes('*')) { - return [target, suffix.replaceAll('*', target)]; - } - return [target, `${target}${suffix}`]; -} - -function isSedExecutable(value: string, shellDialect: 'bash' | 'powershell'): boolean { - const normalized = shellDialect === 'powershell' ? value.toLowerCase() : value; - return /(?:^|[/\\])sed(?:\.exe)?$/.test(normalized) || /^sed\b/.test(normalized); -} - -function isLongOptionAbbreviation(optionName: string, fullName: string, minimumLength: number): boolean { - return optionName.length >= minimumLength && fullName.startsWith(optionName); -} - -function isKnownNoArgumentLongOption(optionName: string): boolean { - return [ - '--debug', - '--help', - '--null-data', - '--posix', - '--quiet', - '--regexp-extended', - '--sandbox', - '--separate', - '--silent', - '--unbuffered', - '--version', - ].includes(optionName); -} - -function tokenizeCommand(commandText: string, shellDialect: 'bash' | 'powershell'): IShellWord[] { - const words: IShellWord[] = []; - let value = ''; - let hasRuntimeExpansion = false; - let quote: '\'' | '"' | undefined; - let escaping = false; - let wordStarted = false; - - const pushWord = () => { - words.push({ value, hasRuntimeExpansion }); - value = ''; - hasRuntimeExpansion = false; - wordStarted = false; - }; - - for (let index = 0; index < commandText.length; index++) { - const char = commandText[index]; - if (escaping) { - wordStarted = true; - if (char !== '\n') { - value += char; - } - escaping = false; - continue; - } - if (char === '\\' && shellDialect === 'bash' && quote !== '\'') { - const next = commandText[index + 1]; - if (quote !== '"' || next === '$' || next === '`' || next === '"' || next === '\\' || next === '\n') { - wordStarted = true; - escaping = true; - continue; - } - } - if (char === quote) { - quote = undefined; - continue; - } - if (!quote && (char === '\'' || char === '"')) { - wordStarted = true; - quote = char; - continue; - } - if (!quote && /\s/.test(char)) { - if (wordStarted) { - pushWord(); - } - continue; - } - if (quote !== '\'' && (char === '$' || char === '`' || (!quote && /[*?[{()}]/.test(char)))) { - hasRuntimeExpansion = true; - } - wordStarted = true; - value += char; - } - if (wordStarted) { - if (quote || escaping) { - hasRuntimeExpansion = true; - } - pushWord(); - } - return words; -} diff --git a/src/vs/platform/terminal/common/sedFileWriteParser.ts b/src/vs/platform/terminal/common/sedFileWriteParser.ts new file mode 100644 index 00000000000000..11ed304322c413 --- /dev/null +++ b/src/vs/platform/terminal/common/sedFileWriteParser.ts @@ -0,0 +1,210 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +/** + * Parser for detecting file writes from `sed` commands using in-place editing. + * + * Handles: + * - `sed -i 's/foo/bar/' file.txt` (GNU) + * - `sed -i.bak 's/foo/bar/' file.txt` (GNU with backup suffix) + * - `sed -i '' 's/foo/bar/' file.txt` (macOS/BSD with empty backup suffix) + * - `sed --in-place 's/foo/bar/' file.txt` (GNU long form) + * - `sed --in-place=.bak 's/foo/bar/' file.txt` (GNU long form with backup) + * - `sed -I 's/foo/bar/' file.txt` (BSD case-insensitive variant) + */ +export class SedFileWriteParser { + readonly commandName = 'sed'; + + canHandle(commandText: string): boolean { + // Check if this is a sed command + if (!commandText.match(/^sed\s+/)) { + return false; + } + + // Check for -i, -I, or --in-place flag + const inPlaceRegex = /(?:^|\s)(-[a-zA-Z]*[iI][a-zA-Z]*\S*|--in-place(?:=\S*)?|(-i|-I)\s*'[^']*'|(-i|-I)\s*"[^"]*")(?:\s|$)/; + return inPlaceRegex.test(commandText); + } + + extractFileWrites(commandText: string): string[] { + const tokens = this._tokenizeCommand(commandText); + return this._extractFileTargets(tokens); + } + + /** + * Tokenizes a command into individual arguments, handling quotes and escapes. + */ + private _tokenizeCommand(commandText: string): string[] { + const tokens: string[] = []; + let current = ''; + let inSingleQuote = false; + let inDoubleQuote = false; + let escaped = false; + + for (let i = 0; i < commandText.length; i++) { + const char = commandText[i]; + + if (escaped) { + current += char; + escaped = false; + continue; + } + + if (char === '\\' && !inSingleQuote) { + escaped = true; + current += char; + continue; + } + + if (char === '\'' && !inDoubleQuote) { + inSingleQuote = !inSingleQuote; + current += char; + continue; + } + + if (char === '"' && !inSingleQuote) { + inDoubleQuote = !inDoubleQuote; + current += char; + continue; + } + + if (/\s/.test(char) && !inSingleQuote && !inDoubleQuote) { + if (current) { + tokens.push(current); + current = ''; + } + continue; + } + + current += char; + } + + if (current) { + tokens.push(current); + } + + return tokens; + } + + /** + * Extracts file targets from tokenized sed command arguments. + * Files are generally the last non-option, non-script arguments. + */ + private _extractFileTargets(tokens: string[]): string[] { + if (tokens.length === 0 || tokens[0] !== 'sed') { + return []; + } + + const files: string[] = []; + let i = 1; // Skip 'sed' + let foundScript = false; + + while (i < tokens.length) { + const token = tokens[i]; + + // Long options + if (token.startsWith('--')) { + if (token === '--in-place' || token.startsWith('--in-place=')) { + // In-place flag (already verified we have one) + i++; + continue; + } + if (token === '--expression' || token === '--file') { + // Skip the option and its argument + i += 2; + foundScript = true; + continue; + } + if (token.startsWith('--expression=') || token.startsWith('--file=')) { + i++; + foundScript = true; + continue; + } + // Other long options like --sandbox, --debug, etc. + i++; + continue; + } + + // Short options + if (token.startsWith('-') && token.length > 1 && token[1] !== '-') { + // Could be combined flags like -ni or -i.bak + const flags = token.slice(1); + + // Check if this is -i with backup suffix attached (e.g., -i.bak) + const iIndex = flags.indexOf('i'); + const IIndex = flags.indexOf('I'); + const inPlaceIndex = iIndex >= 0 ? iIndex : IIndex; + + if (inPlaceIndex >= 0 && inPlaceIndex < flags.length - 1) { + // -i.bak style - backup suffix is attached + i++; + continue; + } + + // Check if -i or -I is the last flag and next token could be backup suffix + if ((flags.endsWith('i') || flags.endsWith('I')) && i + 1 < tokens.length) { + const nextToken = tokens[i + 1]; + // macOS/BSD style: -i '' or -i "" (empty string backup suffix) + // Only treat it as a backup suffix if it's empty or looks like a backup + // extension (starts with '.' and is short). Don't match sed scripts like 's/foo/bar/'. + if (nextToken === '\'\'' || nextToken === '""') { + i += 2; + continue; + } + // Check for quoted backup suffixes like '.bak' or ".backup" + if ((nextToken.startsWith('\'') && nextToken.endsWith('\'')) || (nextToken.startsWith('"') && nextToken.endsWith('"'))) { + const unquoted = nextToken.slice(1, -1); + // Backup suffixes typically start with '.' and are short extensions + if (unquoted.startsWith('.') && unquoted.length <= 10 && !unquoted.includes('/')) { + i += 2; + continue; + } + } + } + + // Check for -e or -f which take arguments + if (flags.includes('e') || flags.includes('f')) { + const eIndex = flags.indexOf('e'); + const fIndex = flags.indexOf('f'); + const optIndex = eIndex >= 0 ? eIndex : fIndex; + + // If -e or -f is not the last character, the rest of the token is the argument + if (optIndex < flags.length - 1) { + foundScript = true; + i++; + continue; + } + + // Otherwise, the next token is the argument + foundScript = true; + i += 2; + continue; + } + + i++; + continue; + } + + // Non-option argument + if (!foundScript) { + // First non-option is the script (unless -e/-f was used) + foundScript = true; + i++; + continue; + } + + // Subsequent non-option arguments are files + // Strip surrounding quotes from file path + let file = token; + if ((file.startsWith('\'') && file.endsWith('\'')) || (file.startsWith('"') && file.endsWith('"'))) { + file = file.slice(1, -1); + } + files.push(file); + i++; + } + + return files; + } +} diff --git a/src/vs/platform/terminal/test/common/sedCommandAnalyzer.test.ts b/src/vs/platform/terminal/test/common/sedCommandAnalyzer.test.ts deleted file mode 100644 index aa0097a88983c6..00000000000000 --- a/src/vs/platform/terminal/test/common/sedCommandAnalyzer.test.ts +++ /dev/null @@ -1,146 +0,0 @@ -/*--------------------------------------------------------------------------------------------- - * Copyright (c) Microsoft Corporation. All rights reserved. - * Licensed under the MIT License. See License.txt in the project root for license information. - *--------------------------------------------------------------------------------------------*/ - -import assert from 'assert'; -import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js'; -import { analyzeSedCommand } from '../../common/sedCommandAnalyzer.js'; - -suite('analyzeSedCommand', () => { - - ensureNoDisposablesAreLeakedInTestSuite(); - - test('allows literal non-in-place commands', () => { - const commands = [ - 'echo sed -i file.txt', - 'sed "s/foo/bar/" file.txt', - 'sed -n "s/foo/bar/p" file.txt', - 'sed -E "s/(foo)/bar/" file.txt', - 'sed --quiet "s/foo/bar/p" file.txt', - 'sed --sandbox "s/foo/bar/" file.txt', - 'sed -- "$SED_OPTIONS" file.txt', - 'sed "s/foo/bar/" "-\\inside.txt"', - ]; - - assert.deepStrictEqual( - commands.map(command => analyzeSedCommand(command)), - commands.map(() => ({ kind: 'safe' })), - ); - }); - - test('identifies static semantic in-place options', () => { - const commands = [ - 'sed -i "s/foo/bar/" file.txt', - 'sed -I .bak "s/foo/bar/" file.txt', - 'sed -ni "s/foo/bar/" file.txt', - 'sed -n -i "s/foo/bar/" file.txt', - 'sed -i.bak "s/foo/bar/" file.txt', - 'sed -i \'\' "s/foo/bar/" file.txt', - 'sed --in-place "s/foo/bar/" file.txt', - 'sed --in-place=.bak "s/foo/bar/" file.txt', - 'sed --in-plac "s/foo/bar/" file.txt', - 'sed.exe -i "s/foo/bar/" file.txt', - 'sed "-i" "s/foo/bar/" file.txt', - 'sed -\\i "s/foo/bar/" file.txt', - 'sed "s/foo/bar/" -inside.txt', - 'sed -i\'../outside/*\' "s/foo/bar/" file.txt', - 'sed --follow-symlinks -i "s/foo/bar/" link.txt', - ]; - - assert.deepStrictEqual( - commands.map(command => analyzeSedCommand(command).kind), - [ - 'inPlace', - 'inPlace', - 'inPlace', - 'inPlace', - 'inPlace', - 'inPlace', - 'inPlace', - 'inPlace', - 'inPlace', - 'inPlace', - 'inPlace', - 'inPlace', - 'requiresConfirmation', - 'inPlace', - 'requiresConfirmation', - ], - ); - }); - - test('requires confirmation for runtime-resolved option words', () => { - const commands = [ - 'sed "$SED_OPTIONS" "s/foo/bar/" file.txt', - 'sed "$(echo --in-place)" "s/foo/bar/" file.txt', - 'sed "s/foo/bar/" "$SED_OPTIONS" file.txt', - 'sed "s/foo/bar/" "$(echo --in-place)" file.txt', - 'sed -i "s/foo/bar/" *.txt', - 'sed${PATH:+} -i "s/foo/bar/" file.txt', - 'sed${PATH:+} "s/foo/bar/" file.txt', - ]; - - assert.deepStrictEqual( - commands.map(command => analyzeSedCommand(command)), - commands.map(() => ({ kind: 'requiresConfirmation' })), - ); - }); - - test('extracts the union of static GNU and BSD write destinations', () => { - const cases = [ - ['sed -i "s/foo/bar/" file.txt', ['file.txt']], - ['sed -i.bak "s/foo/bar/" file.txt', ['file.txt', 'file.txt.bak']], - ['sed --in-place "s/foo/bar/" file.txt', ['file.txt']], - ['sed --in-place=.bak "s/foo/bar/" file.txt', ['file.txt', 'file.txt.bak']], - ['sed -i "" "s/foo/bar/" file.txt', ['s/foo/bar/', 'file.txt']], - ['sed -i json "s/foo/bar/" package.', ['s/foo/bar/', 'package.', 'package.json']], - ['sed -I .json "s/foo/bar/" package', ['package', 'package.json']], - ['sed -i\'../outside/*\' "s/foo/bar/" inside.txt', ['inside.txt', '../outside/inside.txt', 'inside.txt../outside/*']], - ['sed -i "s/foo/bar/" file1.txt file2.txt', ['file1.txt', 'file2.txt', 'file2.txts/foo/bar/']], - ['sed --in-place -e "s/foo/bar/" file.txt', ['file.txt']], - ['sed -i -x "s/foo/bar/" file.txt', ['file.txt', 'file.txt-x']], - ] as const; - - assert.deepStrictEqual( - cases.map(([commandLine]) => analyzeSedCommand(commandLine)), - cases.map(([, fileWrites]) => ({ kind: 'inPlace', fileWrites: [...fileWrites] })), - ); - }); - - test('decodes backslashes according to the shell dialect', () => { - assert.deepStrictEqual({ - bashUnquoted: analyzeSedCommand('sed --in-place "s/foo/bar/" \\/etc/config', 'bash'), - bashDoubleQuotedLiteral: analyzeSedCommand('sed --in-place "s/foo/bar/" "path\\q"', 'bash'), - bashDoubleQuotedEscapedExpansion: analyzeSedCommand('sed --in-place "s/foo/bar/" "path\\$FILE"', 'bash'), - bashDoubleQuotedWindowsPath: analyzeSedCommand('sed --in-place "s/foo/bar/" "C:\\outside\\file.txt"', 'bash'), - powerShellPath: analyzeSedCommand('sed --in-place "s/foo/bar/" C:\\outside\\file.txt', 'powershell'), - powerShellUppercase: analyzeSedCommand('SED -i "s/foo/bar/" file.txt', 'powershell'), - powerShellUppercaseExe: analyzeSedCommand('SED.EXE -i "s/foo/bar/" file.txt', 'powershell'), - }, { - bashUnquoted: { kind: 'inPlace', fileWrites: ['/etc/config'] }, - bashDoubleQuotedLiteral: { kind: 'inPlace', fileWrites: ['path\\q'] }, - bashDoubleQuotedEscapedExpansion: { kind: 'inPlace', fileWrites: ['path$FILE'] }, - bashDoubleQuotedWindowsPath: { kind: 'inPlace', fileWrites: ['C:\\outside\\file.txt'] }, - powerShellPath: { kind: 'inPlace', fileWrites: ['C:\\outside\\file.txt'] }, - powerShellUppercase: { kind: 'inPlace', fileWrites: ['file.txt'] }, - powerShellUppercaseExe: { kind: 'inPlace', fileWrites: ['file.txt'] }, - }); - }); - - test('requires confirmation when static destinations cannot be determined', () => { - const commands = [ - 'sed -i "s/foo/bar/"', - 'sed --follow-symlinks -i "s/foo/bar/" link.txt', - 'sed --in-place --expr="s/foo/bar/" outside.txt', - 'sed --in-place --fi=script.sed outside.txt', - 'sed -i.bak "-e" $ARGS inside.txt', - 'sed --in-place --file "$SCRIPT" inside.txt', - ]; - - assert.deepStrictEqual( - commands.map(command => analyzeSedCommand(command)), - commands.map(() => ({ kind: 'requiresConfirmation' })), - ); - }); -}); diff --git a/src/vs/platform/terminal/test/common/sedFileWriteParser.test.ts b/src/vs/platform/terminal/test/common/sedFileWriteParser.test.ts new file mode 100644 index 00000000000000..4e547ec417130e --- /dev/null +++ b/src/vs/platform/terminal/test/common/sedFileWriteParser.test.ts @@ -0,0 +1,49 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from 'assert'; +import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js'; +import { SedFileWriteParser } from '../../common/sedFileWriteParser.js'; + +suite('SedFileWriteParser', () => { + + ensureNoDisposablesAreLeakedInTestSuite(); + + const parser = new SedFileWriteParser(); + + test('detects supported in-place options', () => { + const commandLines = [ + 'sed -i "s/foo/bar/" file.txt', + 'sed -I "s/foo/bar/" file.txt', + 'sed -ni "s/foo/bar/" file.txt', + 'sed -i.bak "s/foo/bar/" file.txt', + 'sed -i \'\' "s/foo/bar/" file.txt', + 'sed --in-place "s/foo/bar/" file.txt', + 'sed --in-place=.bak "s/foo/bar/" file.txt', + ]; + assert.deepStrictEqual(commandLines.map(commandLine => parser.canHandle(commandLine)), commandLines.map(() => true)); + }); + + test('does not classify non-in-place commands', () => { + const commandLines = [ + 'sed "s/foo/bar/" file.txt', + 'sed -n "s/foo/bar/p" file.txt', + 'echo sed -i file.txt', + ]; + assert.deepStrictEqual(commandLines.map(commandLine => parser.canHandle(commandLine)), commandLines.map(() => false)); + }); + + test('extracts in-place file targets', () => { + assert.deepStrictEqual({ + single: parser.extractFileWrites('sed -i "s/foo/bar/" file.txt'), + multiple: parser.extractFileWrites('sed -i "s/foo/bar/" file1.txt file2.txt'), + bsd: parser.extractFileWrites('sed -i \'\' "s/foo/bar/" file.txt'), + }, { + single: ['file.txt'], + multiple: ['file1.txt', 'file2.txt'], + bsd: ['file.txt'], + }); + }); +}); diff --git a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/commandParsers/commandFileWriteParser.ts b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/commandParsers/commandFileWriteParser.ts index d521ac9e0b8e44..09d14d75e422d2 100644 --- a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/commandParsers/commandFileWriteParser.ts +++ b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/commandParsers/commandFileWriteParser.ts @@ -25,8 +25,7 @@ export interface ICommandFileWriteParser { * Extracts the file paths that would be written to by this command. * Should only be called if canHandle() returns true. * @param commandText The full text of a single command (not a pipeline). - * @returns Array of file paths that would be modified. An undefined entry - * indicates a write whose destination cannot be determined statically. + * @returns Array of file paths that would be modified. */ - extractFileWrites(commandText: string): (string | undefined)[]; + extractFileWrites(commandText: string): string[]; } diff --git a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/commandParsers/sedFileWriteParser.ts b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/commandParsers/sedFileWriteParser.ts deleted file mode 100644 index 58e69b62f04f53..00000000000000 --- a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/commandParsers/sedFileWriteParser.ts +++ /dev/null @@ -1,34 +0,0 @@ -/*--------------------------------------------------------------------------------------------- - * Copyright (c) Microsoft Corporation. All rights reserved. - * Licensed under the MIT License. See License.txt in the project root for license information. - *--------------------------------------------------------------------------------------------*/ - -import { ICommandFileWriteParser } from './commandFileWriteParser.js'; -import { analyzeSedCommand } from '../../../../../../platform/terminal/common/sedCommandAnalyzer.js'; - -/** - * Parser for detecting file writes from `sed` commands using in-place editing. - * - * Handles: - * - `sed -i 's/foo/bar/' file.txt` (GNU) - * - `sed -i.bak 's/foo/bar/' file.txt` (GNU with backup suffix) - * - `sed -i '' 's/foo/bar/' file.txt` (macOS/BSD with empty backup suffix) - * - `sed --in-place 's/foo/bar/' file.txt` (GNU long form) - * - `sed --in-place=.bak 's/foo/bar/' file.txt` (GNU long form with backup) - * - `sed -I 's/foo/bar/' file.txt` (BSD case-insensitive variant) - */ -export class SedFileWriteParser implements ICommandFileWriteParser { - readonly commandName = 'sed'; - - canHandle(commandText: string): boolean { - return analyzeSedCommand(commandText, 'bash').kind !== 'safe'; - } - - extractFileWrites(commandText: string): (string | undefined)[] { - const analysis = analyzeSedCommand(commandText, 'bash'); - if (analysis.kind === 'requiresConfirmation') { - return [undefined]; - } - return analysis.kind === 'inPlace' ? [...analysis.fileWrites] : []; - } -} diff --git a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/commandLineAnalyzer/commandLineFileWriteAnalyzer.ts b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/commandLineAnalyzer/commandLineFileWriteAnalyzer.ts index 08810e94f7431b..c30f6a83009592 100644 --- a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/commandLineAnalyzer/commandLineFileWriteAnalyzer.ts +++ b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/commandLineAnalyzer/commandLineFileWriteAnalyzer.ts @@ -19,7 +19,7 @@ import { ILabelService } from '../../../../../../../platform/label/common/label. const nullDevice = Symbol('null device'); -type FileWrite = URI | string | typeof nullDevice | undefined; +type FileWrite = URI | string | typeof nullDevice; export class CommandLineFileWriteAnalyzer extends Disposable implements ICommandLineAnalyzer { constructor( @@ -64,7 +64,7 @@ export class CommandLineFileWriteAnalyzer extends Disposable implements ICommand if (cwd) { this._log('Detected cwd', cwd.toString()); fileWrites = allCapturedFileWrites.map(e => { - if (e === nullDevice || e === undefined) { + if (e === nullDevice) { return e; } @@ -92,7 +92,7 @@ export class CommandLineFileWriteAnalyzer extends Disposable implements ICommand fileWrites = allCapturedFileWrites; } } - this._log('File writes detected', fileWrites.map(e => e?.toString() ?? 'unknown')); + this._log('File writes detected', fileWrites.map(e => e.toString())); return fileWrites; } @@ -107,10 +107,7 @@ export class CommandLineFileWriteAnalyzer extends Disposable implements ICommand return result; } - private _mapNullDevice(options: ICommandLineAnalyzerOptions, rawFileWrite: string | undefined): string | typeof nullDevice | undefined { - if (rawFileWrite === undefined) { - return undefined; - } + private _mapNullDevice(options: ICommandLineAnalyzerOptions, rawFileWrite: string): string | typeof nullDevice { if (options.treeSitterLanguage === TreeSitterCommandParserLanguage.PowerShell) { return rawFileWrite === '$null' ? nullDevice @@ -135,11 +132,6 @@ export class CommandLineFileWriteAnalyzer extends Disposable implements ICommand const workspaceFolders = this._workspaceContextService.getWorkspace().folders; if (workspaceFolders.length > 0) { for (const fileWrite of fileWrites) { - if (fileWrite === undefined) { - isAutoApproveAllowed = false; - this._log('File write blocked due to unknown destination'); - break; - } if (fileWrite === nullDevice) { this._log('File write to null device allowed', URI.isUri(fileWrite) ? fileWrite.toString() : fileWrite); continue; @@ -201,7 +193,7 @@ export class CommandLineFileWriteAnalyzer extends Disposable implements ICommand const disclaimers: string[] = []; if (fileWrites.length > 0) { - const fileWritesList = fileWrites.map(fw => `\`${URI.isUri(fw) ? this._labelService.getUriLabel(fw) : fw === nullDevice ? '/dev/null' : fw?.toString() ?? localize('unknownFileWriteDestination', "unknown destination")}\``).join(', '); + const fileWritesList = fileWrites.map(fw => `\`${URI.isUri(fw) ? this._labelService.getUriLabel(fw) : fw === nullDevice ? '/dev/null' : fw.toString()}\``).join(', '); if (!isAutoApproveAllowed) { disclaimers.push(localize('runInTerminal.fileWriteBlockedDisclaimer', 'File write operations detected that cannot be auto approved: {0}', fileWritesList)); } else { diff --git a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/treeSitterCommandParser.ts b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/treeSitterCommandParser.ts index e50fde8a43ba00..545c541f85ecaa 100644 --- a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/treeSitterCommandParser.ts +++ b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/treeSitterCommandParser.ts @@ -11,8 +11,8 @@ import { Disposable, MutableDisposable, toDisposable } from '../../../../../base import { posix, win32 } from '../../../../../base/common/path.js'; import { ITreeSitterLibraryService } from '../../../../../editor/common/services/treeSitter/treeSitterLibraryService.js'; import type { ITerminalSandboxCommand } from '../../../../../platform/sandbox/common/terminalSandboxService.js'; +import { SedFileWriteParser } from '../../../../../platform/terminal/common/sedFileWriteParser.js'; import { ICommandFileWriteParser } from './commandParsers/commandFileWriteParser.js'; -import { SedFileWriteParser } from './commandParsers/sedFileWriteParser.js'; export const enum TreeSitterCommandParserLanguage { Bash = 'bash', @@ -151,7 +151,7 @@ export class TreeSitterCommandParser extends Disposable { * Uses registered command parsers (e.g., for `sed -i`) to detect command-specific file writes. * Returns an array of file paths that would be modified. */ - async getCommandFileWrites(languageId: TreeSitterCommandParserLanguage, commandLine: string): Promise<(string | undefined)[]> { + async getCommandFileWrites(languageId: TreeSitterCommandParserLanguage, commandLine: string): Promise { // Currently only bash-like shells are supported for command-specific parsing if (languageId !== TreeSitterCommandParserLanguage.Bash) { return []; @@ -161,7 +161,7 @@ export class TreeSitterCommandParser extends Disposable { const query = '(command) @command'; const captures = await this._queryTree(languageId, commandLine, query); - const result: (string | undefined)[] = []; + const result: string[] = []; for (const capture of captures) { const commandText = capture.node.text; for (const parser of this._commandFileWriteParsers) { diff --git a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/electron-browser/commandLineAnalyzer/commandLineFileWriteAnalyzer.test.ts b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/electron-browser/commandLineAnalyzer/commandLineFileWriteAnalyzer.test.ts index fb930036370c79..4c881a55cb27d0 100644 --- a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/electron-browser/commandLineAnalyzer/commandLineFileWriteAnalyzer.test.ts +++ b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/electron-browser/commandLineAnalyzer/commandLineFileWriteAnalyzer.test.ts @@ -196,7 +196,7 @@ suite('CommandLineFileWriteAnalyzer', () => { suite('sed in-place editing', () => { // Basic -i flag variants (inside workspace) test('sed -i inside workspace - allow', () => t('sed -i \'s/foo/bar/\' file.txt', 'outsideWorkspace', true, 1)); - test('sed -I (uppercase) inside workspace - allow', () => t('sed -I \'\' \'s/foo/bar/\' file.txt', 'outsideWorkspace', true, 1)); + test('sed -I (uppercase) inside workspace - allow', () => t('sed -I \'s/foo/bar/\' file.txt', 'outsideWorkspace', true, 1)); test('sed --in-place inside workspace - allow', () => t('sed --in-place \'s/foo/bar/\' file.txt', 'outsideWorkspace', true, 1)); // Backup suffix variants (inside workspace) @@ -222,14 +222,6 @@ suite('CommandLineFileWriteAnalyzer', () => { // With blockDetectedFileWrites: never test('sed -i with never setting - allow', () => t('sed -i \'s/foo/bar/\' file.txt', 'never', true, 1)); - // Shared sed analysis fails closed when destinations are ambiguous - test('sed -i missing target - block', () => t('sed -i \'s/foo/bar/\'', 'outsideWorkspace', false, 1)); - test('sed -i glob target - block', () => t('sed -i \'s/foo/bar/\' *.txt', 'outsideWorkspace', false, 1)); - test('sed runtime option - block', () => t('sed "$SED_OPTIONS" \'s/foo/bar/\' file.txt', 'outsideWorkspace', false, 1)); - test('sed runtime expression - block', () => t('sed -i.bak -e "$SCRIPT" file.txt', 'outsideWorkspace', false, 1)); - test('sed --follow-symlinks -i - block', () => t('sed --follow-symlinks -i \'s/foo/bar/\' file.txt', 'outsideWorkspace', false, 1)); - test('sed backup suffix outside workspace - block', () => t('sed -i\'../outside/*\' \'s/foo/bar/\' file.txt', 'outsideWorkspace', false, 1)); - // Without -i flag (should not detect as file write) test('sed without -i - no file write detected', () => t('sed \'s/foo/bar/\' file.txt', 'outsideWorkspace', true, 0)); test('sed with pipe - no file write detected', () => t('cat file.txt | sed \'s/foo/bar/\'', 'outsideWorkspace', true, 0)); From 878bd4cbf42b693f0f1c89c5fa9ae70e034f83f7 Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Mon, 3 Aug 2026 23:34:58 -0700 Subject: [PATCH 6/6] Group shared sed parser under terminal auto-approval Place the shared parser and its tests under terminal/common/autoApprove to make the feature-specific ownership explicit without changing behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bda42f98-e73f-417f-b7e6-03cff9e2f604 --- src/vs/platform/agentHost/node/commandAutoApprover.ts | 2 +- .../terminal/common/{ => autoApprove}/sedFileWriteParser.ts | 0 .../test/common/{ => autoApprove}/sedFileWriteParser.test.ts | 4 ++-- .../chatAgentTools/browser/treeSitterCommandParser.ts | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) rename src/vs/platform/terminal/common/{ => autoApprove}/sedFileWriteParser.ts (100%) rename src/vs/platform/terminal/test/common/{ => autoApprove}/sedFileWriteParser.test.ts (93%) diff --git a/src/vs/platform/agentHost/node/commandAutoApprover.ts b/src/vs/platform/agentHost/node/commandAutoApprover.ts index 2586ddb6703fe8..fb7ddc833a9b82 100644 --- a/src/vs/platform/agentHost/node/commandAutoApprover.ts +++ b/src/vs/platform/agentHost/node/commandAutoApprover.ts @@ -11,7 +11,7 @@ import { escapeRegExpCharacters, regExpLeadsToEndlessLoop } from '../../../base/ import { URI } from '../../../base/common/uri.js'; import { getAppNodeModulesPath } from './appNodeModules.js'; import { ILogService } from '../../log/common/log.js'; -import { SedFileWriteParser } from '../../terminal/common/sedFileWriteParser.js'; +import { SedFileWriteParser } from '../../terminal/common/autoApprove/sedFileWriteParser.js'; import type { AgentHostTerminalAutoApproveRuleValue, AgentHostTerminalAutoApproveRules } from '../common/agentHostSchema.js'; /** diff --git a/src/vs/platform/terminal/common/sedFileWriteParser.ts b/src/vs/platform/terminal/common/autoApprove/sedFileWriteParser.ts similarity index 100% rename from src/vs/platform/terminal/common/sedFileWriteParser.ts rename to src/vs/platform/terminal/common/autoApprove/sedFileWriteParser.ts diff --git a/src/vs/platform/terminal/test/common/sedFileWriteParser.test.ts b/src/vs/platform/terminal/test/common/autoApprove/sedFileWriteParser.test.ts similarity index 93% rename from src/vs/platform/terminal/test/common/sedFileWriteParser.test.ts rename to src/vs/platform/terminal/test/common/autoApprove/sedFileWriteParser.test.ts index 4e547ec417130e..fb4d8c4256d19e 100644 --- a/src/vs/platform/terminal/test/common/sedFileWriteParser.test.ts +++ b/src/vs/platform/terminal/test/common/autoApprove/sedFileWriteParser.test.ts @@ -4,8 +4,8 @@ *--------------------------------------------------------------------------------------------*/ import assert from 'assert'; -import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js'; -import { SedFileWriteParser } from '../../common/sedFileWriteParser.js'; +import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; +import { SedFileWriteParser } from '../../../common/autoApprove/sedFileWriteParser.js'; suite('SedFileWriteParser', () => { diff --git a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/treeSitterCommandParser.ts b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/treeSitterCommandParser.ts index 545c541f85ecaa..dfc8623bc8e1e7 100644 --- a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/treeSitterCommandParser.ts +++ b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/treeSitterCommandParser.ts @@ -11,7 +11,7 @@ import { Disposable, MutableDisposable, toDisposable } from '../../../../../base import { posix, win32 } from '../../../../../base/common/path.js'; import { ITreeSitterLibraryService } from '../../../../../editor/common/services/treeSitter/treeSitterLibraryService.js'; import type { ITerminalSandboxCommand } from '../../../../../platform/sandbox/common/terminalSandboxService.js'; -import { SedFileWriteParser } from '../../../../../platform/terminal/common/sedFileWriteParser.js'; +import { SedFileWriteParser } from '../../../../../platform/terminal/common/autoApprove/sedFileWriteParser.js'; import { ICommandFileWriteParser } from './commandParsers/commandFileWriteParser.js'; export const enum TreeSitterCommandParserLanguage {