From b1fef3b5ec66495ed7de2b111a7b4f57e1d6c6e3 Mon Sep 17 00:00:00 2001 From: Aishanee Shah Date: Thu, 5 Mar 2026 21:23:58 +0000 Subject: [PATCH] fix(core): enhance webfetch security and reliability based on PR feedback --- packages/core/src/tools/web-fetch.test.ts | 32 ++++++++++---- packages/core/src/tools/web-fetch.ts | 54 ++++++++++++++++++----- 2 files changed, 67 insertions(+), 19 deletions(-) diff --git a/packages/core/src/tools/web-fetch.test.ts b/packages/core/src/tools/web-fetch.test.ts index bf92d0a7d6..f3d5723563 100644 --- a/packages/core/src/tools/web-fetch.test.ts +++ b/packages/core/src/tools/web-fetch.test.ts @@ -419,6 +419,7 @@ describe('WebFetchTool', () => { expect(result.llmContent).toContain( '[Warning] The following URLs were skipped due to rate limiting: https://ratelimit-multi.com/', ); + expect(result.returnDisplay).toContain('(1 URL(s) skipped)'); }); it('should rescue failed public URLs via fallback', async () => { @@ -494,7 +495,10 @@ describe('WebFetchTool', () => { expect(result.llmContent).toContain('public content'); expect(result.llmContent).not.toContain('--- Rescued Content ---'); - expect(result.llmContent).not.toContain('URL: https://private.com/'); + expect(result.llmContent).toContain( + '[Warning] The following URLs were skipped because they point to private IP addresses: https://private.com/', + ); + expect(result.returnDisplay).toContain('(1 URL(s) skipped)'); }); it('should return WEB_FETCH_FALLBACK_FAILED on fallback fetch failure', async () => { @@ -520,13 +524,6 @@ describe('WebFetchTool', () => { it('should log telemetry when falling back due to private IP', async () => { vi.spyOn(fetchUtils, 'isPrivateIp').mockReturnValue(true); - // Mock fetchWithTimeout to succeed so fallback proceeds - mockFetch('https://private.ip/', { - text: () => Promise.resolve('some content'), - }); - mockGenerateContent.mockResolvedValue({ - candidates: [{ content: { parts: [{ text: 'fallback response' }] } }], - }); const tool = new WebFetchTool(mockConfig, bus); const params = { prompt: 'fetch https://private.ip' }; @@ -537,7 +534,9 @@ describe('WebFetchTool', () => { mockConfig, expect.any(WebFetchFallbackAttemptEvent), ); - expect(WebFetchFallbackAttemptEvent).toHaveBeenCalledWith('private_ip'); + expect(WebFetchFallbackAttemptEvent).toHaveBeenCalledWith( + 'private_ip_skipped', + ); }); it('should log telemetry when falling back due to primary fetch failure', async () => { @@ -1074,5 +1073,20 @@ describe('WebFetchTool', () => { expect(result.llmContent).toContain('Error: Invalid URL "not-a-url"'); expect(result.error?.type).toBe(ToolErrorType.INVALID_TOOL_PARAMS); }); + + it('should block private IP (experimental)', async () => { + vi.spyOn(fetchUtils, 'isPrivateIp').mockReturnValue(true); + const tool = new WebFetchTool(mockConfig, bus); + const invocation = tool['createInvocation']( + { url: 'http://localhost' }, + bus, + ); + const result = await invocation.execute(new AbortController().signal); + + expect(result.llmContent).toContain( + 'Error: Access to private IP address http://localhost/ is not allowed.', + ); + expect(result.error?.type).toBe(ToolErrorType.WEB_FETCH_PROCESSING_ERROR); + }); }); }); diff --git a/packages/core/src/tools/web-fetch.ts b/packages/core/src/tools/web-fetch.ts index c34df69364..f6304acbd6 100644 --- a/packages/core/src/tools/web-fetch.ts +++ b/packages/core/src/tools/web-fetch.ts @@ -224,6 +224,9 @@ class WebFetchToolInvocation extends BaseToolInvocation< contentBudget: number, ): Promise { const url = convertGithubUrlToRaw(urlStr); + if (isPrivateIp(url)) { + return `Error fetching ${url}: Access to private IP address is not allowed.`; + } try { const response = await retryWithBackoff( @@ -444,6 +447,18 @@ ${aggregatedContent} // Convert GitHub blob URL to raw URL url = convertGithubUrlToRaw(url); + if (isPrivateIp(url)) { + const errorMessage = `Access to private IP address ${url} is not allowed.`; + return { + llmContent: `Error: ${errorMessage}`, + returnDisplay: `Error: ${errorMessage}`, + error: { + message: errorMessage, + type: ToolErrorType.WEB_FETCH_PROCESSING_ERROR, + }, + }; + } + try { const response = await retryWithBackoff( async () => { @@ -600,7 +615,7 @@ Response: ${truncateString(rawResponseText, 10000, '\n\n... [Error response trun if (privateUrls.length > 0) { logWebFetchFallbackAttempt( this.config, - new WebFetchFallbackAttemptEvent('private_ip'), + new WebFetchFallbackAttemptEvent('private_ip_skipped'), ); } @@ -619,9 +634,14 @@ Response: ${truncateString(rawResponseText, 10000, '\n\n... [Error response trun ); let responseText = getResponseText(response) || ''; - const urlContextMeta = response.candidates?.[0]?.urlContextMetadata as - | UrlContextMetadata - | undefined; + const rawUrlContextMeta = response.candidates?.[0]?.urlContextMetadata; + const urlContextMeta = + rawUrlContextMeta && + typeof rawUrlContextMeta === 'object' && + 'urlMetadata' in rawUrlContextMeta && + Array.isArray(rawUrlContextMeta.urlMetadata) + ? (rawUrlContextMeta as UrlContextMetadata) + : undefined; const groundingMetadata = response.candidates?.[0]?.groundingMetadata; const sources = groundingMetadata?.groundingChunks as | GroundingChunkItem[] @@ -749,13 +769,27 @@ Response: ${truncateString(rawResponseText, 10000, '\n\n... [Error response trun } } - // Unit 2: Append rate limiting warning + // Unit 2: Append rate limiting and private IP warnings + const warnings: string[] = []; if (rateLimited.length > 0) { - const warning = `[Warning] The following URLs were skipped due to rate limiting: ${rateLimited.join( - ', ', - )}`; - llmContent = `${warning}\n\n${llmContent}`; - returnDisplay = `${returnDisplay} (Warning: ${rateLimited.length} URL(s) rate-limited)`; + warnings.push( + `[Warning] The following URLs were skipped due to rate limiting: ${rateLimited.join( + ', ', + )}`, + ); + } + if (privateUrls.length > 0) { + warnings.push( + `[Warning] The following URLs were skipped because they point to private IP addresses: ${privateUrls.join( + ', ', + )}`, + ); + } + + if (warnings.length > 0) { + const combinedWarning = warnings.join('\n'); + llmContent = `${combinedWarning}\n\n${llmContent}`; + returnDisplay = `${returnDisplay} (${rateLimited.length + privateUrls.length} URL(s) skipped)`; } return {