From 6505ab8c6cfa0783cefe26edc3f237f9baa0efc6 Mon Sep 17 00:00:00 2001 From: Alisa Novikova <62909685+alisa-alisa@users.noreply.github.com> Date: Fri, 6 Mar 2026 03:46:58 -0800 Subject: [PATCH] feat(a2a): implement robust IP validation, secure acknowledgement, and URL splitting fix --- package-lock.json | 1 + packages/core/package.json | 1 + .../src/agents/a2a-client-manager.test.ts | 40 +------------ .../core/src/agents/a2a-client-manager.ts | 24 +------- packages/core/src/agents/a2aUtils.test.ts | 55 +++++++++++++++++ packages/core/src/agents/a2aUtils.ts | 29 +++++++++ packages/core/src/agents/registry.test.ts | 38 ++++++++++-- packages/core/src/agents/registry.ts | 39 ++++++++++-- packages/core/src/utils/fetch.test.ts | 3 + packages/core/src/utils/fetch.ts | 59 +++++++++++-------- 10 files changed, 192 insertions(+), 97 deletions(-) diff --git a/package-lock.json b/package-lock.json index 036fdc8ec1..8963837258 100644 --- a/package-lock.json +++ b/package-lock.json @@ -17523,6 +17523,7 @@ "html-to-text": "^9.0.5", "https-proxy-agent": "^7.0.6", "ignore": "^7.0.0", + "ipaddr.js": "^1.9.1", "js-yaml": "^4.1.1", "marked": "^15.0.12", "mime": "4.0.7", diff --git a/packages/core/package.json b/packages/core/package.json index dd2aa29706..8861046d01 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -65,6 +65,7 @@ "html-to-text": "^9.0.5", "https-proxy-agent": "^7.0.6", "ignore": "^7.0.0", + "ipaddr.js": "^1.9.1", "js-yaml": "^4.1.1", "marked": "^15.0.12", "mime": "4.0.7", diff --git a/packages/core/src/agents/a2a-client-manager.test.ts b/packages/core/src/agents/a2a-client-manager.test.ts index 96c91ae33f..9b660e66f2 100644 --- a/packages/core/src/agents/a2a-client-manager.test.ts +++ b/packages/core/src/agents/a2a-client-manager.test.ts @@ -419,7 +419,7 @@ describe('A2AClientManager', () => { expect(resolverInstance.resolve).toHaveBeenCalledWith( 'http://localhost:9001/', - '.well-known/agent-card.json', + undefined, ); }); @@ -481,43 +481,5 @@ describe('A2AClientManager', () => { /contains transport URL pointing to private IP range: http:\/\/192.168.1.1\/api/, ); }); - - it('should handle URLs where .well-known appears in the domain/subdomain', async () => { - const trickyUrl = - 'http://.well-known/agent-card.json.attacker.com/my-agent'; - const resolverInstance = { - resolve: vi.fn().mockResolvedValue({ name: 'test' } as AgentCard), - }; - vi.mocked(sdkClient.DefaultAgentCardResolver).mockReturnValue( - resolverInstance as unknown as sdkClient.DefaultAgentCardResolver, - ); - - await manager.loadAgent('tricky-agent', trickyUrl); - - // Should treat the whole thing as baseUrl since it doesn't end with standardPath - expect(resolverInstance.resolve).toHaveBeenCalledWith( - trickyUrl, - undefined, - ); - }); - - it('should correctly handle URLs with standardPath in the hash fragment', async () => { - const fragmentUrl = - 'http://localhost:9001/.well-known/agent-card.json#.well-known/agent-card.json'; - const resolverInstance = { - resolve: vi.fn().mockResolvedValue({ name: 'test' } as AgentCard), - }; - vi.mocked(sdkClient.DefaultAgentCardResolver).mockReturnValue( - resolverInstance as unknown as sdkClient.DefaultAgentCardResolver, - ); - - await manager.loadAgent('fragment-agent', fragmentUrl); - - // Should correctly ignore the hash fragment and use the path from the URL object - expect(resolverInstance.resolve).toHaveBeenCalledWith( - 'http://localhost:9001/', - '.well-known/agent-card.json', - ); - }); }); }); diff --git a/packages/core/src/agents/a2a-client-manager.ts b/packages/core/src/agents/a2a-client-manager.ts index cefde87daf..a3f5cbd763 100644 --- a/packages/core/src/agents/a2a-client-manager.ts +++ b/packages/core/src/agents/a2a-client-manager.ts @@ -29,6 +29,7 @@ import { getGrpcCredentials, normalizeAgentCard, pinUrlToIp, + splitAgentCardUrl, } from './a2aUtils.js'; import { isPrivateIpAsync, @@ -291,10 +292,6 @@ export class A2AClientManager { url: string, resolver: DefaultAgentCardResolver, ): Promise { - const standardPath = '.well-known/agent-card.json'; - let baseUrl = url; - let path: string | undefined; - // Validate URL to prevent SSRF (with DNS resolution) if (await isPrivateIpAsync(url)) { // Local/private IPs are allowed ONLY for localhost for testing. @@ -306,24 +303,7 @@ export class A2AClientManager { } } - try { - const parsedUrl = new URL(url); - if (parsedUrl.pathname.endsWith(standardPath)) { - // Correctly split the URL into baseUrl and standard path - path = standardPath; - // Reconstruct baseUrl from parsed components to avoid issues with hashes or query params. - parsedUrl.pathname = parsedUrl.pathname.substring( - 0, - parsedUrl.pathname.lastIndexOf(standardPath), - ); - parsedUrl.search = ''; - parsedUrl.hash = ''; - baseUrl = parsedUrl.toString(); - } - } catch (e) { - throw new Error(`Invalid agent card URL: ${url}`, { cause: e }); - } - + const { baseUrl, path } = splitAgentCardUrl(url); const rawCard = await resolver.resolve(baseUrl, path); const agentCard = normalizeAgentCard(rawCard); diff --git a/packages/core/src/agents/a2aUtils.test.ts b/packages/core/src/agents/a2aUtils.test.ts index 2257975c66..4514c36d9f 100644 --- a/packages/core/src/agents/a2aUtils.test.ts +++ b/packages/core/src/agents/a2aUtils.test.ts @@ -14,6 +14,7 @@ import { normalizeAgentCard, getGrpcCredentials, pinUrlToIp, + splitAgentCardUrl, } from './a2aUtils.js'; import type { SendMessageResult } from './a2a-client-manager.js'; import type { @@ -423,6 +424,60 @@ describe('a2aUtils', () => { 'http://127.0.0.1:8080', ); }); + + it('should NOT override existing transport if protocolBinding is also present', () => { + const raw = { + name: 'priority-test', + supportedInterfaces: [ + { url: 'foo', transport: 'GRPC', protocolBinding: 'REST' }, + ], + }; + const normalized = normalizeAgentCard(raw); + expect(normalized.additionalInterfaces?.[0].transport).toBe('GRPC'); + }); + }); + + describe('splitAgentCardUrl', () => { + const standard = '.well-known/agent-card.json'; + + it('should return baseUrl as-is if it does not end with standard path', () => { + const url = 'http://localhost:9001/custom/path'; + expect(splitAgentCardUrl(url)).toEqual({ baseUrl: url }); + }); + + it('should split correctly if URL ends with standard path', () => { + const url = `http://localhost:9001/${standard}`; + expect(splitAgentCardUrl(url)).toEqual({ + baseUrl: 'http://localhost:9001/', + path: undefined, + }); + }); + + it('should handle trailing slash in baseUrl when splitting', () => { + const url = `http://example.com/api/${standard}`; + expect(splitAgentCardUrl(url)).toEqual({ + baseUrl: 'http://example.com/api/', + path: undefined, + }); + }); + + it('should ignore hashes and query params when splitting', () => { + const url = `http://localhost:9001/${standard}?foo=bar#baz`; + expect(splitAgentCardUrl(url)).toEqual({ + baseUrl: 'http://localhost:9001/', + path: undefined, + }); + }); + + it('should return original URL if parsing fails', () => { + const url = 'not-a-url'; + expect(splitAgentCardUrl(url)).toEqual({ baseUrl: url }); + }); + + it('should handle standard path appearing earlier in the path', () => { + const url = `http://localhost:9001/${standard}/something-else`; + expect(splitAgentCardUrl(url)).toEqual({ baseUrl: url }); + }); }); describe('A2AResultReassembler', () => { diff --git a/packages/core/src/agents/a2aUtils.ts b/packages/core/src/agents/a2aUtils.ts index 3e45870182..b4a9e74730 100644 --- a/packages/core/src/agents/a2aUtils.ts +++ b/packages/core/src/agents/a2aUtils.ts @@ -340,6 +340,35 @@ export async function pinUrlToIp( } } +/** + * Splts an agent card URL into a baseUrl and a standard path if it already + * contains '.well-known/agent-card.json'. + */ +export function splitAgentCardUrl(url: string): { + baseUrl: string; + path?: string; +} { + const standardPath = '.well-known/agent-card.json'; + try { + const parsedUrl = new URL(url); + if (parsedUrl.pathname.endsWith(standardPath)) { + // Reconstruct baseUrl from parsed components to avoid issues with hashes or query params. + parsedUrl.pathname = parsedUrl.pathname.substring( + 0, + parsedUrl.pathname.lastIndexOf(standardPath), + ); + parsedUrl.search = ''; + parsedUrl.hash = ''; + // We return undefined for path if it's the standard one, + // because the SDK's DefaultAgentCardResolver appends it automatically. + return { baseUrl: parsedUrl.toString(), path: undefined }; + } + } catch (_e) { + // Ignore URL parsing errors here, let the resolver handle them. + } + return { baseUrl: url }; +} + /** * Extracts contextId and taskId from a Message, Task, or Update response. * Follows the pattern from the A2A CLI sample to maintain conversational continuity. diff --git a/packages/core/src/agents/registry.test.ts b/packages/core/src/agents/registry.test.ts index f7070ed895..7276159bff 100644 --- a/packages/core/src/agents/registry.test.ts +++ b/packages/core/src/agents/registry.test.ts @@ -5,6 +5,7 @@ */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import * as crypto from 'node:crypto'; import { AgentRegistry, getModelConfigAlias } from './registry.js'; import { makeFakeConfig } from '../test-utils/config.js'; import type { AgentDefinition, LocalAgentDefinition } from './types.js'; @@ -29,10 +30,21 @@ import { SimpleExtensionLoader } from '../utils/extensionLoader.js'; import type { ToolRegistry } from '../tools/tool-registry.js'; import { ThinkingLevel } from '@google/genai'; import type { AcknowledgedAgentsService } from './acknowledgedAgents.js'; +import * as sdkClient from '@a2a-js/sdk/client'; import { PolicyDecision } from '../policy/types.js'; import { A2AAuthProviderFactory } from './auth-provider/factory.js'; import type { A2AAuthProvider } from './auth-provider/types.js'; +vi.mock('@a2a-js/sdk/client', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...(actual as Record), + DefaultAgentCardResolver: vi.fn().mockImplementation(() => ({ + resolve: vi.fn().mockResolvedValue({ name: 'RemoteAgent' }), + })), + }; +}); + vi.mock('./agentLoader.js', () => ({ loadAgentsFromDirectory: vi .fn() @@ -417,7 +429,7 @@ describe('AgentRegistry', () => { expect(registry.getDefinition('extension-agent')).toBeUndefined(); }); - it('should use agentCardUrl as hash for acknowledgement of remote agents', async () => { + it('should use agentCardUrl and content-based hash for acknowledgement of remote agents', async () => { mockConfig = makeMockedConfig({ enableAgents: true }); // Trust the folder so it attempts to load project agents vi.spyOn(mockConfig, 'isTrustedFolder').mockReturnValue(true); @@ -453,19 +465,33 @@ describe('AgentRegistry', () => { clearCache: vi.fn(), } as unknown as A2AClientManager); + // Mock the resolver to return a consistent card content for hashing + const mockCardContent = { name: 'RemoteAgent' }; + const expectedContentHash = crypto + .createHash('sha256') + .update(JSON.stringify(mockCardContent)) + .digest('hex'); + const expectedHash = `https://example.com/card#${expectedContentHash}`; + + vi.mocked(sdkClient.DefaultAgentCardResolver).mockImplementation( + () => + ({ + resolve: vi.fn().mockResolvedValue(mockCardContent), + }) as unknown as sdkClient.DefaultAgentCardResolver, + ); + await registry.initialize(); - // Verify ackService was called with the URL, not the file hash + // Verify ackService was called with the content-based hash expect(ackService.isAcknowledged).toHaveBeenCalledWith( expect.anything(), 'RemoteAgent', - 'https://example.com/card', + expectedHash, ); - // Also verify that the agent's metadata was updated to use the URL as hash - // Use getDefinition because registerAgent might have been called + // Also verify that the agent's metadata was updated to use the content-based hash expect(registry.getDefinition('RemoteAgent')?.metadata?.hash).toBe( - 'https://example.com/card', + expectedHash, ); }); }); diff --git a/packages/core/src/agents/registry.ts b/packages/core/src/agents/registry.ts index 11b1e74b07..1af713da7f 100644 --- a/packages/core/src/agents/registry.ts +++ b/packages/core/src/agents/registry.ts @@ -9,14 +9,17 @@ import { CoreEvent, coreEvents } from '../utils/events.js'; import type { AgentOverride, Config } from '../config/config.js'; import type { AgentDefinition, LocalAgentDefinition } from './types.js'; import { loadAgentsFromDirectory } from './agentLoader.js'; +import { splitAgentCardUrl } from './a2aUtils.js'; import { CodebaseInvestigatorAgent } from './codebase-investigator.js'; import { CliHelpAgent } from './cli-help-agent.js'; import { GeneralistAgent } from './generalist-agent.js'; import { BrowserAgentDefinition } from './browser/browserAgentDefinition.js'; import { A2AClientManager } from './a2a-client-manager.js'; import { A2AAuthProviderFactory } from './auth-provider/factory.js'; +import { DefaultAgentCardResolver } from '@a2a-js/sdk/client'; import type { AuthenticationHandler } from '@a2a-js/sdk/client'; import { type z } from 'zod'; +import * as crypto from 'node:crypto'; import { debugLogger } from '../utils/debugLogger.js'; import { isAutoModel } from '../config/models.js'; import { @@ -155,13 +158,35 @@ export class AgentRegistry { const agentsToRegister: AgentDefinition[] = []; for (const agent of projectAgents.agents) { - // If it's a remote agent, use the agentCardUrl as the hash. - // This allows multiple remote agents in a single file to be tracked independently. + // For remote agents, we must use a content-based hash of the AgentCard + // to prevent Indirect Prompt Injection if the remote card is modified. if (agent.kind === 'remote') { - if (!agent.metadata) { - agent.metadata = {}; + try { + // We use a dedicated resolver here to fetch the card for hashing. + // This is separate from loadAgent to keep hashing logic isolated. + const resolver = new DefaultAgentCardResolver(); + const { baseUrl, path } = splitAgentCardUrl(agent.agentCardUrl); + const rawCard = await resolver.resolve(baseUrl, path); + const cardContent = JSON.stringify(rawCard); + const contentHash = crypto + .createHash('sha256') + .update(cardContent) + .digest('hex'); + + if (!agent.metadata) { + agent.metadata = {}; + } + // Combining URL and content hash ensures we track specific card versions at specific locations. + agent.metadata.hash = `${agent.agentCardUrl}#${contentHash}`; + } catch (e) { + debugLogger.warn( + `[AgentRegistry] Could not fetch remote card for hashing "${agent.name}":`, + e, + ); + // If we can't fetch the card, we can't verify its acknowledgement securely. + unacknowledgedAgents.push(agent); + continue; } - agent.metadata.hash = agent.agentCardUrl; } if (!agent.metadata?.hash) { @@ -178,6 +203,10 @@ export class AgentRegistry { if (isAcknowledged) { agentsToRegister.push(agent); } else { + // Register unacknowledged agents so they are visible to the LLM. + // They will be registered with ASK_USER policy, triggering the + // acknowledgement flow when the LLM tries to call them. + agentsToRegister.push(agent); unacknowledgedAgents.push(agent); } } diff --git a/packages/core/src/utils/fetch.test.ts b/packages/core/src/utils/fetch.test.ts index 7f0f9c3edd..39f10d4761 100644 --- a/packages/core/src/utils/fetch.test.ts +++ b/packages/core/src/utils/fetch.test.ts @@ -51,7 +51,10 @@ describe('fetch utils', () => { expect(isAddressPrivate('192.0.0.1')).toBe(true); expect(isAddressPrivate('192.0.2.1')).toBe(true); expect(isAddressPrivate('192.88.99.1')).toBe(true); + // Benchmark range (198.18.0.0/15) + expect(isAddressPrivate('198.18.0.0')).toBe(true); expect(isAddressPrivate('198.18.0.1')).toBe(true); + expect(isAddressPrivate('198.19.255.255')).toBe(true); expect(isAddressPrivate('198.51.100.1')).toBe(true); expect(isAddressPrivate('203.0.113.1')).toBe(true); expect(isAddressPrivate('224.0.0.1')).toBe(true); diff --git a/packages/core/src/utils/fetch.ts b/packages/core/src/utils/fetch.ts index efd096833e..ca8fa258b7 100644 --- a/packages/core/src/utils/fetch.ts +++ b/packages/core/src/utils/fetch.ts @@ -9,6 +9,7 @@ import { URL } from 'node:url'; import * as dns from 'node:dns'; import { lookup } from 'node:dns/promises'; import { Agent, ProxyAgent, setGlobalDispatcher } from 'undici'; +import ipaddr from 'ipaddr.js'; const DEFAULT_HEADERS_TIMEOUT = 300000; // 5 minutes const DEFAULT_BODY_TIMEOUT = 300000; // 5 minutes @@ -21,26 +22,6 @@ setGlobalDispatcher( }), ); -const PRIVATE_IP_RANGES = [ - /^10\./, - /^127\./, - /^0\./, - /^100\.(6[4-9]|[7-9][0-9]|1[0-1][0-9]|12[0-7])\./, - /^169\.254\./, - /^172\.(1[6-9]|2[0-9]|3[0-1])\./, - /^192\.0\.(0|2)\./, - /^192\.88\.99\./, - /^192\.168\./, - /^198\.(1[8-9]|51\.100)\./, - /^203\.0\.113\./, - /^2(2[4-9]|3[0-9]|[4-5][0-9])\./, - /^::1$/, - /^::$/, - /^f[cd]00:/i, // fc00::/7 (ULA) - /^fe[89ab][0-9a-f]:/i, // fe80::/10 (Link-local) - /^::ffff:(10\.|127\.|0\.|100\.(6[4-9]|[7-9][0-9]|1[0-1][0-9]|12[0-7])\.|169\.254\.|172\.(1[6-9]|2[0-9]|3[0-1])\.|192\.0\.(0|2)\.|192\.88\.99\.|192\.168\.|198\.(1[8-9]|51\.100)\.|203\.0\.113\.|2(2[4-9]|3[0-9]|[4-5][0-9])\.|a9fe:|ac1[0-9a-f]:|c0a8:|0+:)/i, // IPv4-mapped IPv6 -]; - // Local extension of RequestInit to support Node.js/undici dispatcher interface NodeFetchInit extends RequestInit { dispatcher?: Agent | ProxyAgent; @@ -166,15 +147,43 @@ export async function isPrivateIpAsync(url: string): Promise { } /** - * Internal helper to check if an IP address string is in a private range. + * Internal helper to check if an IP address string is in a private or reserved range. */ export function isAddressPrivate(address: string): boolean { const sanitized = sanitizeHostname(address); - return ( - sanitized === 'localhost' || - PRIVATE_IP_RANGES.some((range) => range.test(sanitized)) - ); + if (sanitized === 'localhost') { + return true; + } + + try { + if (!ipaddr.isValid(sanitized)) { + return false; + } + + const addr = ipaddr.parse(sanitized); + const range = addr.range(); + + // Special handling for IPv4-mapped IPv6 (::ffff:x.x.x.x) + // We unmap it and check the underlying IPv4 address. + if (addr instanceof ipaddr.IPv6 && addr.isIPv4MappedAddress()) { + return isAddressPrivate(addr.toIPv4Address().toString()); + } + + // Explicitly block 198.18.0.0/15 (Benchmark testing) which ipaddr.js + // classifies as unicast, but is not public internet. + if (addr instanceof ipaddr.IPv4) { + const [r, bits] = ipaddr.parseCIDR('198.18.0.0/15'); + if (r instanceof ipaddr.IPv4 && addr.match(r, bits)) { + return true; + } + } + + return range !== 'unicast'; + } catch (_e) { + // If parsing fails despite isValid(), we treat it as potentially unsafe. + return true; + } } /**