fix(core): enhance webfetch security and reliability based on PR feedback

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);
+    });
   });
 });

packages/core/src/tools/web-fetch.ts

@@ -224,6 +224,9 @@ class WebFetchToolInvocation extends BaseToolInvocation<
     contentBudget: number,
   ): Promise<string> {
     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 {