From e428f03c19c563fd774214fdaa15216fe13a3463 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Sun, 22 Mar 2026 00:51:11 +0000 Subject: [PATCH] fix: fetch tool should not warn when reading files inside the workspace (#303789) --- .../builtInTools/fetchPageTool.ts | 24 ++-- .../tools/builtinTools/fetchPageTool.test.ts | 118 +++++++++++++++++- 2 files changed, 134 insertions(+), 8 deletions(-) diff --git a/src/vs/workbench/contrib/chat/electron-browser/builtInTools/fetchPageTool.ts b/src/vs/workbench/contrib/chat/electron-browser/builtInTools/fetchPageTool.ts index d59f63292d7..e1b59c7a695 100644 --- a/src/vs/workbench/contrib/chat/electron-browser/builtInTools/fetchPageTool.ts +++ b/src/vs/workbench/contrib/chat/electron-browser/builtInTools/fetchPageTool.ts @@ -13,6 +13,7 @@ import { URI } from '../../../../../base/common/uri.js'; import { localize } from '../../../../../nls.js'; import { IFileService } from '../../../../../platform/files/common/files.js'; import { IWebContentExtractorService, WebContentExtractResult } from '../../../../../platform/webContentExtractor/common/webContentExtractor.js'; +import { IWorkspaceContextService } from '../../../../../platform/workspace/common/workspace.js'; import { detectEncodingFromBuffer } from '../../../../services/textfile/common/encoding.js'; import { ITrustedDomainService } from '../../../url/browser/trustedDomainService.js'; import { IChatService } from '../../common/chatService/chatService.js'; @@ -56,6 +57,7 @@ export class FetchWebPageTool implements IToolImpl { @IFileService private readonly _fileService: IFileService, @ITrustedDomainService private readonly _trustedDomainService: ITrustedDomainService, @IChatService private readonly _chatService: IChatService, + @IWorkspaceContextService private readonly _workspaceContextService: IWorkspaceContextService, ) { } async invoke(invocation: IToolInvocation, _countTokens: CountTokensCallback, _progress: ToolProgress, token: CancellationToken): Promise { @@ -170,7 +172,15 @@ export class FetchWebPageTool implements IToolImpl { } const invalid = [...Array.from(invalidUris), ...additionalInvalidUrls]; - const urlsNeedingConfirmation = new ResourceSet([...webUris.values(), ...validFileUris]); + // All valid URIs (web + file) for display in messages + const allFetchedUris = new ResourceSet([...webUris.values(), ...validFileUris]); + // File URIs that are inside the workspace don't need confirmation — they're already accessible + // and don't carry the web content risks (prompt injection, malicious redirects). + // File URIs outside the workspace are treated like web URIs and require confirmation. + const fileUrisOutsideWorkspace = validFileUris.filter( + uri => !this._workspaceContextService.getWorkspaceFolder(uri) + ); + const urlsNeedingConfirmation = new ResourceSet([...webUris.values(), ...fileUrisOutsideWorkspace]); const pastTenseMessage = invalid.length ? invalid.length > 1 @@ -178,7 +188,7 @@ export class FetchWebPageTool implements IToolImpl { ? new MarkdownString( localize( 'fetchWebPage.pastTenseMessage.plural', - 'Fetched {0} resources, but the following were invalid URLs:\n\n{1}\n\n', urlsNeedingConfirmation.size, invalid.map(url => `- ${url}`).join('\n') + 'Fetched {0} resources, but the following were invalid URLs:\n\n{1}\n\n', allFetchedUris.size, invalid.map(url => `- ${url}`).join('\n') )) // If there is only one invalid URL, show it : new MarkdownString( @@ -190,11 +200,11 @@ export class FetchWebPageTool implements IToolImpl { : new MarkdownString(); const invocationMessage = new MarkdownString(); - if (urlsNeedingConfirmation.size > 1) { - pastTenseMessage.appendMarkdown(localize('fetchWebPage.pastTenseMessageResult.plural', 'Fetched {0} resources', urlsNeedingConfirmation.size)); - invocationMessage.appendMarkdown(localize('fetchWebPage.invocationMessage.plural', 'Fetching {0} resources', urlsNeedingConfirmation.size)); - } else if (urlsNeedingConfirmation.size === 1) { - const url = Iterable.first(urlsNeedingConfirmation)!.toString(true); + if (allFetchedUris.size > 1) { + pastTenseMessage.appendMarkdown(localize('fetchWebPage.pastTenseMessageResult.plural', 'Fetched {0} resources', allFetchedUris.size)); + invocationMessage.appendMarkdown(localize('fetchWebPage.invocationMessage.plural', 'Fetching {0} resources', allFetchedUris.size)); + } else if (allFetchedUris.size === 1) { + const url = Iterable.first(allFetchedUris)!.toString(true); // If the URL is too long or it's a file url, show it as a link... otherwise, show it as plain text if (url.length > 400 || validFileUris.length === 1) { pastTenseMessage.appendMarkdown(localize({ diff --git a/src/vs/workbench/contrib/chat/test/electron-browser/tools/builtinTools/fetchPageTool.test.ts b/src/vs/workbench/contrib/chat/test/electron-browser/tools/builtinTools/fetchPageTool.test.ts index 12819789846..a47b17723ce 100644 --- a/src/vs/workbench/contrib/chat/test/electron-browser/tools/builtinTools/fetchPageTool.test.ts +++ b/src/vs/workbench/contrib/chat/test/electron-browser/tools/builtinTools/fetchPageTool.test.ts @@ -11,8 +11,9 @@ import { ResourceMap } from '../../../../../../../base/common/map.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../../../base/test/common/utils.js'; import { IFileContent, IReadFileOptions } from '../../../../../../../platform/files/common/files.js'; import { IWebContentExtractorService, WebContentExtractResult } from '../../../../../../../platform/webContentExtractor/common/webContentExtractor.js'; +import { testWorkspace } from '../../../../../../../platform/workspace/test/common/testWorkspace.js'; import { FetchWebPageTool } from '../../../../electron-browser/builtInTools/fetchPageTool.js'; -import { TestFileService } from '../../../../../../test/common/workbenchTestServices.js'; +import { TestContextService, TestFileService } from '../../../../../../test/common/workbenchTestServices.js'; import { MockTrustedDomainService } from '../../../../../url/test/browser/mockTrustedDomainService.js'; import { InternalFetchWebPageToolId } from '../../../../common/tools/builtinTools/tools.js'; import { MockChatService } from '../../../common/chatService/mockChatService.js'; @@ -91,6 +92,7 @@ suite('FetchWebPageTool', () => { new ExtendedTestFileService(fileContentMap), new MockTrustedDomainService(), new MockChatService(), + new TestContextService(), ); const testUrls = [ @@ -140,6 +142,7 @@ suite('FetchWebPageTool', () => { new ExtendedTestFileService(new ResourceMap()), new MockTrustedDomainService([]), new MockChatService(), + new TestContextService(), ); // Test empty array @@ -189,6 +192,7 @@ suite('FetchWebPageTool', () => { new ExtendedTestFileService(fileContentMap), new MockTrustedDomainService(), new MockChatService(), + new TestContextService(), ); const preparation = await tool.prepareToolInvocation( @@ -203,6 +207,100 @@ suite('FetchWebPageTool', () => { assert.ok(messageText.includes('invalid://invalid'), 'Should mention invalid URL'); }); + test('should not show confirmation dialog for file URIs inside the workspace', async () => { + // Use a workspace rooted at /workspaceRoot + const workspaceRoot = URI.file('/workspaceRoot'); + const workspaceContextService = new TestContextService(testWorkspace(workspaceRoot)); + + const fileContentMap = new ResourceMap([ + [URI.file('/workspaceRoot/plan.md'), 'Plan content'], + [URI.file('/workspaceRoot/subdir/notes.txt'), 'Notes content'], + ]); + + const tool = new FetchWebPageTool( + new TestWebContentExtractorService(new ResourceMap()), + new ExtendedTestFileService(fileContentMap), + new MockTrustedDomainService([]), + new MockChatService(), + workspaceContextService, + ); + + // File inside workspace - should NOT trigger confirmation + const preparation = await tool.prepareToolInvocation( + { parameters: { urls: [URI.file('/workspaceRoot/plan.md').toString()] }, toolCallId: 'test-file-in-ws', chatSessionResource: undefined }, + CancellationToken.None + ); + assert.ok(preparation, 'Should return prepared invocation'); + assert.strictEqual(preparation.confirmationMessages?.title, undefined, 'File inside workspace should not show confirmation dialog'); + assert.strictEqual(preparation.confirmationMessages?.confirmResults, false, 'File inside workspace should not require post-confirmation'); + }); + + test('should show confirmation dialog for file URIs outside the workspace', async () => { + // Use a workspace rooted at /workspaceRoot + const workspaceRoot = URI.file('/workspaceRoot'); + const workspaceContextService = new TestContextService(testWorkspace(workspaceRoot)); + + const fileContentMap = new ResourceMap([ + [URI.file('/tmp/external-plan.md'), 'External plan content'], + ]); + + const tool = new FetchWebPageTool( + new TestWebContentExtractorService(new ResourceMap()), + new ExtendedTestFileService(fileContentMap), + new MockTrustedDomainService([]), + new MockChatService(), + workspaceContextService, + ); + + // File outside workspace - should still trigger confirmation + const preparation = await tool.prepareToolInvocation( + { parameters: { urls: [URI.file('/tmp/external-plan.md').toString()] }, toolCallId: 'test-file-outside-ws', chatSessionResource: undefined }, + CancellationToken.None + ); + assert.ok(preparation, 'Should return prepared invocation'); + assert.ok(preparation.confirmationMessages?.title, 'File outside workspace should show confirmation dialog'); + assert.strictEqual(preparation.confirmationMessages?.confirmResults, true, 'File outside workspace should require post-confirmation'); + }); + + test('workspace file mixed with untrusted web URI: only web URI triggers confirmation', async () => { + const workspaceRoot = URI.file('/workspaceRoot'); + const workspaceContextService = new TestContextService(testWorkspace(workspaceRoot)); + + const webContentMap = new ResourceMap([ + [URI.parse('https://example.com'), 'Web content'] + ]); + const fileContentMap = new ResourceMap([ + [URI.file('/workspaceRoot/plan.md'), 'Plan content'] + ]); + + const tool = new FetchWebPageTool( + new TestWebContentExtractorService(webContentMap), + new ExtendedTestFileService(fileContentMap), + new MockTrustedDomainService([]), // No trusted domains + new MockChatService(), + workspaceContextService, + ); + + // Mix: one untrusted web URI + one workspace file URI + const preparation = await tool.prepareToolInvocation( + { + parameters: { urls: ['https://example.com', URI.file('/workspaceRoot/plan.md').toString()] }, + toolCallId: 'test-mixed', + chatSessionResource: undefined + }, + CancellationToken.None + ); + assert.ok(preparation, 'Should return prepared invocation'); + // Confirmation should only be for the web URI + assert.ok(preparation.confirmationMessages?.title, 'Should show confirmation for untrusted web URI'); + // The confirmation message should mention only the web URI, not the workspace file + const msgValue = typeof preparation.confirmationMessages?.message === 'string' + ? preparation.confirmationMessages.message + : preparation.confirmationMessages?.message?.value ?? ''; + assert.ok(!msgValue.includes('/workspaceRoot/'), 'Confirmation message should not mention workspace file'); + assert.ok(msgValue.includes('example.com'), 'Confirmation message should mention web URI'); + }); + test('should approve when all URLs were mentioned in chat', async () => { const webContentMap = new ResourceMap([ [URI.parse('https://valid.com'), 'Valid content'] @@ -227,6 +325,7 @@ suite('FetchWebPageTool', () => { }; }, }), + new TestContextService(), ); const preparation1 = await tool.prepareToolInvocation( @@ -261,6 +360,7 @@ suite('FetchWebPageTool', () => { new ExtendedTestFileService(fileContentMap), new MockTrustedDomainService(), new MockChatService(), + new TestContextService(), ); const result = await tool.invoke( @@ -309,6 +409,7 @@ suite('FetchWebPageTool', () => { new ExtendedTestFileService(fileContentMap), new MockTrustedDomainService(), new MockChatService(), + new TestContextService(), ); const result = await tool.invoke( @@ -350,6 +451,7 @@ suite('FetchWebPageTool', () => { new ExtendedTestFileService(fileContentMap), new MockTrustedDomainService(), new MockChatService(), + new TestContextService(), ); const result = await tool.invoke( @@ -397,6 +499,7 @@ suite('FetchWebPageTool', () => { new ExtendedTestFileService(fileContentMap), new MockTrustedDomainService(), new MockChatService(), + new TestContextService(), ); const result = await tool.invoke( @@ -463,6 +566,7 @@ suite('FetchWebPageTool', () => { new ExtendedTestFileService(fileContentMap), new MockTrustedDomainService(), new MockChatService(), + new TestContextService(), ); const result = await tool.invoke( @@ -503,6 +607,7 @@ suite('FetchWebPageTool', () => { new ExtendedTestFileService(fileContentMap), new MockTrustedDomainService(), new MockChatService(), + new TestContextService(), ); const result = await tool.invoke( @@ -547,6 +652,7 @@ suite('FetchWebPageTool', () => { new ExtendedTestFileService(fileContentMap), new MockTrustedDomainService(), new MockChatService(), + new TestContextService(), ); const testUrls = [ @@ -606,6 +712,7 @@ suite('FetchWebPageTool', () => { new ExtendedTestFileService(new ResourceMap()), new MockTrustedDomainService([]), new MockChatService(), + new TestContextService(), ); const testUrls = [ @@ -642,6 +749,7 @@ suite('FetchWebPageTool', () => { new ExtendedTestFileService(fileContentMap), new MockTrustedDomainService(), new MockChatService(), + new TestContextService(), ); const testUrls = [ @@ -684,6 +792,7 @@ suite('FetchWebPageTool', () => { new ExtendedTestFileService(fileContentMap), new MockTrustedDomainService(), new MockChatService(), + new TestContextService(), ); const testUrls = [ @@ -732,6 +841,7 @@ suite('FetchWebPageTool', () => { new ExtendedTestFileService(new ResourceMap()), // Empty - all file , new MockTrustedDomainService([]), new MockChatService(), + new TestContextService(), ); const testUrls = [ @@ -766,6 +876,7 @@ suite('FetchWebPageTool', () => { new ExtendedTestFileService(new ResourceMap()), new MockTrustedDomainService([]), new MockChatService(), + new TestContextService(), ); const result = await tool.invoke( @@ -792,6 +903,7 @@ suite('FetchWebPageTool', () => { new ExtendedTestFileService(fileContentMap), new MockTrustedDomainService(), new MockChatService(), + new TestContextService(), ); const result = await tool.invoke( @@ -829,6 +941,7 @@ suite('FetchWebPageTool', () => { new ExtendedTestFileService(new ResourceMap()), new MockTrustedDomainService(), new MockChatService(), + new TestContextService(), ); const result = await tool.invoke( @@ -856,6 +969,7 @@ suite('FetchWebPageTool', () => { new ExtendedTestFileService(new ResourceMap()), new MockTrustedDomainService(), new MockChatService(), + new TestContextService(), ); const result = await tool.invoke( @@ -888,6 +1002,7 @@ suite('FetchWebPageTool', () => { new ExtendedTestFileService(new ResourceMap()), new MockTrustedDomainService(), new MockChatService(), + new TestContextService(), ); const result = await tool.invoke( @@ -914,6 +1029,7 @@ suite('FetchWebPageTool', () => { new ExtendedTestFileService(new ResourceMap()), new MockTrustedDomainService(), new MockChatService(), + new TestContextService(), ); const result = await tool.invoke(