Ignore file-like domain suffixes in terminal sandbox#307780
Ignore file-like domain suffixes in terminal sandbox#307780dileepyavan merged 2 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts the terminal sandbox’s domain detection to reduce false positives by ignoring hostname-like matches whose final label resembles a common file extension, and adds regressions to ensure typical filename-containing commands remain sandboxed.
Changes:
- Add a file-extension suffix filter to domain normalization to ignore filename-like “domains”.
- Add tests covering filename-like tokens (eg
bundle.js,README.md) so they don’t trigger blocked-domain prompts. - Add additional test cases for common commands that include filenames.
Show a summary per file
| File | Description |
|---|---|
src/vs/workbench/contrib/terminalContrib/chatAgentTools/common/terminalSandboxService.ts |
Adds a suffix-based filter during domain normalization to drop filename-like matches. |
src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/terminalSandboxService.test.ts |
Adds regressions ensuring commands with common filename extensions don’t surface blocked domains. |
Copilot's findings
Comments suppressed due to low confidence (2)
src/vs/workbench/contrib/terminalContrib/chatAgentTools/common/terminalSandboxService.ts:583
- The file-extension filter is implemented inside
_normalizeDomain, which is also used to normalize user-configured allow/deny patterns (_matchesDomainPattern) and domains extracted from explicit URLs/ssh remotes. This couples a command-parsing heuristic (avoid filename false positives) to configuration validation and URL parsing, and can unexpectedly reject otherwise-valid allow/deny patterns or ignore URLs.
To keep behavior scoped, consider moving this check to _extractDomains only for _hostRegex matches (bare hostname-like tokens), or add a parameter so URL/setting normalization isn’t affected.
// Disallow patterns that look like file names with common extensions, as these are unlikely to be intended as network domains and may be false positives from the regex.
const lastLabel = host.slice(host.lastIndexOf('.') + 1);
if (TerminalSandboxService._fileExtensionSuffixes.has(lastLabel)) {
return undefined;
}
return hasWildcardPrefix ? `*.${host}` : host;
src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/terminalSandboxService.test.ts:441
- The
.enventry in this test doesn’t actually exercise the new domain-suffix filtering:.envwon’t match the domain regexes because there’s no label before the dot. As written, this assertion would pass even without the PR’s change.
Consider replacing it with a filename that does match the hostname regex (eg config.env) so the test validates the intended behavior.
const commands = [
'node server.js',
'php index.php',
'java -jar app.java',
'cat styles.css',
'cat README.md',
'cat .env',
];
- Files reviewed: 2/2 changed files
- Comments generated: 2
src/vs/workbench/contrib/terminalContrib/chatAgentTools/common/terminalSandboxService.ts
Show resolved
Hide resolved
...workbench/contrib/terminalContrib/chatAgentTools/test/browser/terminalSandboxService.test.ts
Show resolved
Hide resolved
| private static readonly _urlRegex = /(?:https?|wss?):\/\/[^\s'"`|&;<>]+/gi; | ||
| private static readonly _sshRemoteRegex = /(?:^|[\s'"`])(?:[^\s@:'"`]+@)?([a-zA-Z0-9.-]+\.[a-zA-Z]{2,})(?::[^\s'"`|&;<>]+)(?=$|[\s'"`|&;<>])/gi; | ||
| private static readonly _hostRegex = /(?:^|[\s'"`(=])([a-zA-Z0-9.-]+\.[a-zA-Z]{2,})(?::\d+)?(?=(?:\/[^\s'"`|&;<>]*)?(?:$|[\s'"`)\]|,;|&<>]))/gi; | ||
| private static readonly _fileExtensionSuffixes = new Set([ |
There was a problem hiding this comment.
nit: it's kinda annoying to maintain a hardcoded list like this (unless we know for sure these are all the cases we might have), maybe down the line we can use regex or something of the sort.
There was a problem hiding this comment.
yeah this is tricky as the valid regex for domain can pick up file names too. This list is to avoid false positives.
Summary
.js,.json,.php,.java,.css,.md, and.envTesting
./scripts/test.sh --run src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/terminalSandboxService.test.tsnode --experimental-strip-types build/hygiene.ts src/vs/workbench/contrib/terminalContrib/chatAgentTools/common/terminalSandboxService.ts src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/terminalSandboxService.test.tsfixes #307510