-
Notifications
You must be signed in to change notification settings - Fork 37.3k
Enabling sandboxing for terminal commands execution through copilot chat. #280236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds sandboxing functionality for terminal command execution through Copilot chat. It introduces a new setting that allows commands run via the "run in terminal" tool to be executed in a sandboxed environment using the @anthropic-ai/sandbox-runtime package (via the srt CLI tool). The feature is designed for Linux and macOS only.
Key Changes:
- Adds
sandboxSettingsResourceto user data profiles to store sandbox configuration files - Introduces a new experimental setting
chat.tools.terminal.sandboxto control sandboxing behavior with filesystem and network restrictions - Implements logic to wrap terminal commands with the
srtsandboxing tool and provides a retry mechanism when sandboxed commands fail
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 31 comments.
Show a summary per file
| File | Description |
|---|---|
src/vs/platform/userDataProfile/common/userDataProfile.ts |
Adds sandboxSettingsResource property to IUserDataProfile interface for storing sandbox configuration |
src/vs/platform/terminal/common/terminal.ts |
Defines sandbox-related interfaces (ISandboxRuntimeConfig, ISandboxTerminalSettings) and adds sandbox properties to shell launch config |
src/vs/workbench/contrib/terminalContrib/chatAgentTools/common/terminalChatAgentToolsConfiguration.ts |
Adds schema for the new chat.tools.terminal.sandbox setting with filesystem and network access controls |
src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/runInTerminalTool.ts |
Main implementation: wraps commands with srt, manages sandbox config files, handles retry without sandboxing |
src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/terminal.chatAgentTools.contribution.ts |
Registers retry action that allows users to re-run failed commands without sandboxing |
src/vs/platform/terminal/node/terminalEnvironment.ts |
Adds shell integration support for sandboxed bash terminals on Windows |
package.json / package-lock.json |
Adds @anthropic-ai/sandbox-runtime dependency (v0.0.13) |
| Test files (multiple) | Updates test fixtures to include the new sandboxSettingsResource property |
.vscode/launch.json |
Adds debugging configuration for ptyhost inspection |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| console.debug('shellLaunchConfig:', shellLaunchConfig); | ||
| const args = shellIntegrationInjection?.newArgs || shellLaunchConfig.args || []; | ||
| await this._throttleKillSpawn(); | ||
| this._logService.trace('node-pty.IPty#spawn', shellLaunchConfig.executable, args, options); | ||
| const ptyProcess = spawn(shellLaunchConfig.executable!, args, options); | ||
| console.debug('Spawned sandboxed pty process with PID:', ptyProcess.pid); |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove console.debug statements before committing. These should be replaced with the proper logging service (this._logService.debug()) which is already available in this class, or removed entirely if they were only used for development debugging.
| console.debug('shellLaunchConfig:', shellLaunchConfig); | |
| const args = shellIntegrationInjection?.newArgs || shellLaunchConfig.args || []; | |
| await this._throttleKillSpawn(); | |
| this._logService.trace('node-pty.IPty#spawn', shellLaunchConfig.executable, args, options); | |
| const ptyProcess = spawn(shellLaunchConfig.executable!, args, options); | |
| console.debug('Spawned sandboxed pty process with PID:', ptyProcess.pid); | |
| this._logService.debug('shellLaunchConfig:', shellLaunchConfig); | |
| const args = shellIntegrationInjection?.newArgs || shellLaunchConfig.args || []; | |
| await this._throttleKillSpawn(); | |
| this._logService.trace('node-pty.IPty#spawn', shellLaunchConfig.executable, args, options); | |
| const ptyProcess = spawn(shellLaunchConfig.executable!, args, options); | |
| this._logService.debug('Spawned sandboxed pty process with PID:', ptyProcess.pid); |
| private readonly _commandLineAnalyzers: ICommandLineAnalyzer[]; | ||
|
|
||
| protected readonly _sessionTerminalAssociations: Map<string, IToolTerminal> = new Map(); | ||
| protected readonly _sessionTerminalAssociations: Map<string, [IToolTerminal]> = new Map(); |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Questionable data structure change. The _sessionTerminalAssociations was changed from Map<string, IToolTerminal> to Map<string, [IToolTerminal]> (a tuple with exactly one element). This seems like an attempt to support multiple terminals per session, but:
- A tuple with one element
[IToolTerminal]is unusual - if multiple terminals are needed, use an arrayIToolTerminal[] - The implementation only ever accesses the first element via
[0], suggesting the tuple structure is not being properly utilized - This creates unnecessary complexity in the code
Consider either:
- Reverting to
Map<string, IToolTerminal>if only one terminal per session is needed - Changing to
Map<string, IToolTerminal[]>if multiple terminals per session are needed, and updating the logic to properly handle multiple terminals
| protected readonly _sessionTerminalAssociations: Map<string, [IToolTerminal]> = new Map(); | |
| protected readonly _sessionTerminalAssociations: Map<string, IToolTerminal> = new Map(); |
| command: string; | ||
| explanation: string; | ||
| isBackground: boolean; | ||
| workspaceFolder?: string; |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused parameter. The workspaceFolder parameter is defined in the IRunInTerminalInputParams interface but:
- It's not included in the tool's input schema (lines 207-228), so it will never be provided by the language model
- It's never read or used anywhere in the implementation
Either remove this parameter, or if it's intended for future use, add it to the input schema and implement the logic to use it.
| workspaceFolder?: string; |
| path: `${workSpaceFolder}/.vscode/sandbox-settings.json` | ||
| }); | ||
|
|
||
| this._createSandboxConfig(sandboxSettingsUri, settings); |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing await keyword. The _createSandboxConfig method is async and returns a Promise, but it's being called without await. This could lead to the configuration file not being created before it's needed. Change to: await this._createSandboxConfig(sandboxSettingsUri, settings);
| private readonly _telemetry: RunInTerminalToolTelemetry; | ||
| private readonly _commandArtifactCollector: TerminalCommandArtifactCollector; | ||
| protected readonly _profileFetcher: TerminalProfileFetcher; | ||
| private _isSandboxed: boolean; |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused field. The _isSandboxed field is set in the constructor but never read anywhere in the code. It should be removed, or if it's needed for future use, the logic should use it instead of calling _isSandBoxedTerminal() repeatedly.
| private _isSandboxed: boolean; |
| const isAutoApproveEnabled = this._configurationService.getValue(TerminalChatAgentToolsSettingId.EnableAutoApprove) === true; | ||
| const isAutoApproveWarningAccepted = this._storageService.getBoolean(TerminalToolConfirmationStorageKeys.TerminalAutoApproveWarningAccepted, StorageScope.APPLICATION, false); | ||
| const isAutoApproveAllowed = isEligibleForAutoApproval && isAutoApproveEnabled && isAutoApproveWarningAccepted; | ||
| const isAutoApproveAllowed = (isEligibleForAutoApproval && isAutoApproveEnabled && isAutoApproveWarningAccepted); |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Redundant parentheses. The extra parentheses around this boolean expression are unnecessary and reduce readability. Simplify to: const isAutoApproveAllowed = isEligibleForAutoApproval && isAutoApproveEnabled && isAutoApproveWarningAccepted;
| const isAutoApproveAllowed = (isEligibleForAutoApproval && isAutoApproveEnabled && isAutoApproveWarningAccepted); | |
| const isAutoApproveAllowed = isEligibleForAutoApproval && isAutoApproveEnabled && isAutoApproveWarningAccepted; |
| content: [{ | ||
| kind: 'text', | ||
| value: resultText.join(''), | ||
| value: resultText.join('') |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Missing comma after closing brace. This code is missing a comma after the closing brace and before the trailing comment. While JavaScript allows this, the codebase style includes trailing commas. Change line 780 to: value: resultText.join(''),
| value: resultText.join('') | |
| value: resultText.join(''), |
| } | ||
| }; | ||
| this._sandboxConfigPath = sandboxSettingsUri.fsPath; | ||
| this._fileService.createFile(sandboxSettingsUri, VSBuffer.fromString(JSON.stringify(sandboxSettings, null, '\t')), { overwrite: true }); |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing await for async file operation. The _fileService.createFile() method returns a Promise that should be awaited. Without awaiting, the file creation may not complete before the path is used, and errors will be silently ignored. Change to: await this._fileService.createFile(...)
| this._fileService.createFile(sandboxSettingsUri, VSBuffer.fromString(JSON.stringify(sandboxSettings, null, '\t')), { overwrite: true }); | |
| await this._fileService.createFile(sandboxSettingsUri, VSBuffer.fromString(JSON.stringify(sandboxSettings, null, '\t')), { overwrite: true }); |
| "update-build-ts-version": "npm install -D typescript@next @typescript/native-preview && (cd build && npm run compile)" | ||
| }, | ||
| "dependencies": { | ||
| "@anthropic-ai/sandbox-runtime": "^0.0.13", |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verify the package version exists. The PR adds @anthropic-ai/sandbox-runtime version ^0.0.13. Since this is a very early version (0.0.x), ensure that:
- This exact version exists and is stable enough for production use
- The package is actively maintained
- The API is stable, as breaking changes are common in 0.0.x versions
- Consider whether pinning to an exact version (without
^) would be more appropriate for such an early-stage dependency
| "@anthropic-ai/sandbox-runtime": "^0.0.13", | |
| "@anthropic-ai/sandbox-runtime": "0.0.13", |
| /*--------------------------------------------------------------------------------------------- | ||
| * Copyright (c) Microsoft Corporation. All rights reserved. | ||
| * Licensed under the MIT License. See License.txt in the project root for license information. | ||
| *--------------------------------------------------------------------------------------------*/ | ||
| import { Disposable } from '../../../../../../../base/common/lifecycle.js'; | ||
| import type { ICommandLineRewriter, ICommandLineRewriterOptions } from './commandLineRewriter.js'; | ||
|
|
||
| export class SandboxCommandLineRewriter extends Disposable implements ICommandLineRewriter { | ||
|
|
||
| rewrite(options: ICommandLineRewriterOptions) { | ||
| if (!options.sandboxed) { | ||
| return undefined; | ||
| } else { | ||
| // For sandboxed terminals, we could add specific command line rewrites here if needed in future. | ||
| return { 'rewritten': `srt "${options.commandLine}"`, 'reasoning': 'appending srt for sandboxed terminal' }; | ||
| } | ||
| } | ||
| } |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused file. The SandboxCommandLineRewriter class is defined but never imported or used anywhere in the codebase. The sandboxing logic is implemented directly in runInTerminalTool.ts at line 542 instead of using this rewriter. Either:
- Remove this file if the rewriter approach was abandoned
- Integrate this rewriter into the command line rewriters list and use it instead of the direct command wrapping at line 542
If keeping the file, note that the implementation at line 15 has the same shell injection vulnerability as mentioned in the other comment - it needs proper escaping.
| /*--------------------------------------------------------------------------------------------- | |
| * Copyright (c) Microsoft Corporation. All rights reserved. | |
| * Licensed under the MIT License. See License.txt in the project root for license information. | |
| *--------------------------------------------------------------------------------------------*/ | |
| import { Disposable } from '../../../../../../../base/common/lifecycle.js'; | |
| import type { ICommandLineRewriter, ICommandLineRewriterOptions } from './commandLineRewriter.js'; | |
| export class SandboxCommandLineRewriter extends Disposable implements ICommandLineRewriter { | |
| rewrite(options: ICommandLineRewriterOptions) { | |
| if (!options.sandboxed) { | |
| return undefined; | |
| } else { | |
| // For sandboxed terminals, we could add specific command line rewrites here if needed in future. | |
| return { 'rewritten': `srt "${options.commandLine}"`, 'reasoning': 'appending srt for sandboxed terminal' }; | |
| } | |
| } | |
| } |
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @bpaseroMatched files:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to learn more what a sandboxSettingsResource is and what the new dependency is @anthropic-ai/sandbox-runtime
Are we really pushing this in on the last day of the iteration?
Sure I can setup a call if needed. Daniel is OOF and I dont think it will go this week. |
e5335e3 to
40882e1
Compare
| @@ -512,6 +530,11 @@ export class RunInTerminalTool extends Disposable implements IToolImpl { | |||
| toolSpecificData.commandLine.toolEdited !== toolSpecificData.commandLine.original | |||
| ); | |||
|
|
|||
| if (this._isSandboxedTerminal()) { | |||
| this._logService.info(`RunInTerminalTool: Sandboxing is enabled, wrapping command with srt.`); | |||
| command = `srt --settings "${this._sandboxConfigPath}" "${command}"`; // sandboxing | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only works if srt happens to already be on the user's path, which is not guaranteed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is part of a npm package that is added as a dependency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is not available on the user path when VS Code is installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, this needs to be available. My testing was working as there was a global install on my machine.
|
|
||
| /** Whether to launch the terminal in a sandboxed environment. */ | ||
| sandboxed?: boolean; | ||
|
|
||
| /** Sandbox settings to use when launching the terminal process in a sandboxed environment. */ | ||
| sandboxSettings?: ISandboxRuntimeConfig; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not clear why these are needed -- it doesn't seem like we actually used them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These slipped from previous changes where the entire process is sandboxed.
| } | ||
| } else { | ||
| // User-level setting: use user profile | ||
| this._sandboxConfigPath = this._userDataProfileService.currentProfile.sandboxSettingsResource.fsPath; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work for remotes where the user's profile is on a different machine than the workspace.
I suggest we PR in support for setting the sandbox config via an environment variable for the SRT library (if we want to commit to using that.) There are some other issues with the file handling here, but switching to an environment variable would make them moot.
| // #region Actions | ||
|
|
||
| // Retry action for sandboxed terminal commands (invoked via command link in toolResultMessage) | ||
| registerAction2(class RetryWithoutSandboxingAction extends Action2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just resends the last request and turns sandboxing off in the terminal commands it makes. The terminal command could have been the 20th turn step in a long agentic request, and users would find restarting the whole last request to be frustrating.
We should instead re-run that individual command and proceed from there. I think this may need API changes to support unless we stall after the request fails and before returning to the model (which is also not good, some command failures are normal and expected). A cheap interim solution would be to generate a request saying something like Rerun the terminal command 'X' and proceed from there
Tyriar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't merge this without a proper review from me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's all these launch.json changes about?
fixes #277286