From fc63f6b692c3f992968ca73c8a50e5c6c0fe64c7 Mon Sep 17 00:00:00 2001 From: "haozhe.yang" Date: Thu, 16 Jul 2026 22:47:21 +0800 Subject: [PATCH 1/5] fix(security): close FetchURL SSRF bypasses and DNS-rebinding window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - resolve hostnames via DNS and reject any target resolving to loopback / RFC1918 / link-local / CGNAT / ULA ranges, including IPv4-mapped IPv6 forms (e.g. localtest.me, [::ffff:7f00:1]) — the static host denylist only matched literals and could be bypassed by crafted domains - follow redirects manually with full per-hop revalidation (10-hop cap) instead of auto-following, so a public URL cannot 302 the fetcher at internal services or cloud metadata endpoints - pin each connection to the DNS answers the check validated (per-hop undici Agent with a pinned lookup), closing the TOCTOU / DNS-rebinding window between the check and the connect; skipped when a proxy is configured or allowPrivateAddresses is set - apply to both agent-core and agent-core-v2 providers, with SSRF / redirect / pinning test coverage --- .../src/app/web/providers/local-fetch-url.ts | 227 ++++++++++--- .../app/web/providers/local-fetch-url.test.ts | 304 ++++++++++++++++++ .../test/app/web/tools/fetch-url.test.ts | 13 +- .../src/tools/providers/local-fetch-url.ts | 246 ++++++++++---- .../tools/providers/local-fetch-url.test.ts | 294 ++++++++++++++++- 5 files changed, 985 insertions(+), 99 deletions(-) create mode 100644 packages/agent-core-v2/test/app/web/providers/local-fetch-url.test.ts diff --git a/packages/agent-core-v2/src/app/web/providers/local-fetch-url.ts b/packages/agent-core-v2/src/app/web/providers/local-fetch-url.ts index 30ae12b0c1..fa37f72762 100644 --- a/packages/agent-core-v2/src/app/web/providers/local-fetch-url.ts +++ b/packages/agent-core-v2/src/app/web/providers/local-fetch-url.ts @@ -1,5 +1,12 @@ +import { lookup as callbackLookup, type LookupAddress, type LookupOptions } from 'node:dns'; +import { lookup } from 'node:dns/promises'; +import { BlockList, isIP, type LookupFunction } from 'node:net'; + import { Readability } from '@mozilla/readability'; import { parseHTML as rawParseHTML } from 'linkedom'; +import { Agent, type Dispatcher } from 'undici'; + +import { isProxyConfigured } from '#/_base/utils/proxy'; import { HttpFetchError, type UrlFetcher, type UrlFetchResult } from '../tools/fetch-url-types'; @@ -20,6 +27,10 @@ const DEFAULT_USER_AGENT = const DEFAULT_MAX_BYTES = 10 * 1024 * 1024; +const MAX_REDIRECT_HOPS = 10; + +const REDIRECT_STATUSES = new Set([301, 302, 303, 307, 308]); + export interface LocalFetchURLProviderOptions { userAgent?: string; fetchImpl?: typeof fetch; @@ -44,14 +55,27 @@ export class LocalFetchURLProvider implements UrlFetcher { url: string, options?: { toolCallId?: string; signal?: AbortSignal }, ): Promise { - assertSafeFetchTarget(url, this.allowPrivateAddresses); - - const response = await this.fetchImpl(url, { - method: 'GET', - headers: { 'User-Agent': this.userAgent }, - signal: options?.signal, - }); + // Pinned Agents are created per redirect hop and closed once the final + // body is consumed, so keep-alive sockets never linger. + const dispatchers: Dispatcher[] = []; + try { + const response = await this.requestWithValidatedRedirects( + url, + options?.signal, + dispatchers, + ); + return await this.readResponse(response); + } finally { + await Promise.all( + dispatchers.map((dispatcher) => + dispatcher.close().catch(() => { + }), + ), + ); + } + } + private async readResponse(response: Response): Promise { if (response.status >= 400) { await response.body?.cancel().catch(() => { }); @@ -88,6 +112,70 @@ export class LocalFetchURLProvider implements UrlFetcher { return { content: this.extractMainContent(body), kind: 'extracted' }; } + /** + * GET `url`, following redirects manually. Every hop re-runs the full + * SSRF check (IP-literal + DNS) before the request goes out — a public + * URL must not be able to bounce the fetcher at an internal address. + * Redirects without a `Location` header are treated as final responses. + */ + private async requestWithValidatedRedirects( + url: string, + signal: AbortSignal | undefined, + dispatchers: Dispatcher[], + ): Promise { + let currentUrl = url; + let redirects = 0; + for (;;) { + const target = await resolveSafeFetchTarget(currentUrl, this.allowPrivateAddresses); + const response = await this.fetchImpl(currentUrl, { + method: 'GET', + headers: { 'User-Agent': this.userAgent }, + signal, + redirect: 'manual', + dispatcher: this.pinnedDispatcherFor(target, dispatchers), + }); + if (!REDIRECT_STATUSES.has(response.status)) return response; + const location = response.headers.get('location'); + if (location === null) return response; + await response.body?.cancel().catch(() => { + }); + if (redirects >= MAX_REDIRECT_HOPS) { + throw new Error( + `Too many redirects while fetching "${url}" (limit ${String(MAX_REDIRECT_HOPS)}).`, + ); + } + redirects += 1; + currentUrl = new URL(location, currentUrl).toString(); + } + } + + /** + * Pin the connection to the addresses the safety check just validated. + * undici resolves the origin again when it connects, so without pinning + * an attacker-controlled DNS could answer the check with a public IP and + * the connect with an internal one (TOCTOU / DNS rebinding). + */ + private pinnedDispatcherFor( + target: SafeFetchTarget, + dispatchers: Dispatcher[], + ): RequestInit['dispatcher'] { + // IP literals (and allowPrivate mode) need no pin — there is no second + // resolution to race. + if (target.addresses === undefined) return undefined; + // With an HTTP/SOCKS proxy configured, origin resolution happens on the + // proxy side; a direct-connect pinned Agent would bypass the proxy + // entirely, so pinning only applies to direct connections. + if (isProxyConfigured(process.env)) return undefined; + const dispatcher = new Agent({ + connect: { lookup: pinnedLookup(target.host, target.addresses) }, + }); + dispatchers.push(dispatcher); + // Compatible at runtime (undici is undici); the two type declarations — + // the package's own and the copy bundled with @types/node's global + // fetch — just can't see each other. + return dispatcher as unknown as RequestInit['dispatcher']; + } + private extractMainContent(html: string): string { const primary = parseHTML(html); try { @@ -123,7 +211,42 @@ export class LocalFetchURLProvider implements UrlFetcher { } } -function assertSafeFetchTarget(url: string, allowPrivate: boolean): void { +// SSRF blocklist: loopback / RFC 1918 / link-local / CGNAT / ULA and "this +// network", for both address families. BlockList.check() maps IPv4-mapped +// IPv6 addresses (e.g. ::ffff:127.0.0.1) onto the IPv4 subnets, so mapped +// literals cannot slip past the v4 rules. +const PRIVATE_ADDRESS_BLOCKLIST = (() => { + const list = new BlockList(); + list.addSubnet('0.0.0.0', 8, 'ipv4'); // "this network" + list.addSubnet('10.0.0.0', 8, 'ipv4'); + list.addSubnet('100.64.0.0', 10, 'ipv4'); // CGNAT + list.addSubnet('127.0.0.0', 8, 'ipv4'); // loopback + list.addSubnet('169.254.0.0', 16, 'ipv4'); // link-local / cloud metadata + list.addSubnet('172.16.0.0', 12, 'ipv4'); + list.addSubnet('192.168.0.0', 16, 'ipv4'); + list.addSubnet('::', 128, 'ipv6'); // unspecified + list.addSubnet('::1', 128, 'ipv6'); // loopback + list.addSubnet('fc00::', 7, 'ipv6'); // ULA + list.addSubnet('fe80::', 10, 'ipv6'); // link-local + return list; +})(); + +function isBlockedAddress(address: string): boolean { + // Link-local addresses may carry a zone id ("fe80::1%en0") — strip it + // before matching. + const normalized = address.split('%', 1)[0] ?? address; + if (isIP(normalized) === 4) return PRIVATE_ADDRESS_BLOCKLIST.check(normalized, 'ipv4'); + return isIP(normalized) === 6 && PRIVATE_ADDRESS_BLOCKLIST.check(normalized, 'ipv6'); +} + +interface SafeFetchTarget { + /** Lowercased hostname with any IPv6 brackets stripped. */ + host: string; + /** Validated DNS answers to pin the connection to — absent when no lookup was needed. */ + addresses?: LookupAddress[]; +} + +async function resolveSafeFetchTarget(url: string, allowPrivate: boolean): Promise { let parsed: URL; try { parsed = new URL(url); @@ -133,45 +256,67 @@ function assertSafeFetchTarget(url: string, allowPrivate: boolean): void { if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') { throw new Error(`Unsupported URL scheme "${parsed.protocol}" — only http(s) allowed.`); } - if (allowPrivate) return; + // URL hostname preserves surrounding `[ ]` for IPv6 literals on some + // Node versions (and not others). Strip them for uniform comparison. const hostRaw = parsed.hostname.toLowerCase(); const host = hostRaw.startsWith('[') && hostRaw.endsWith(']') ? hostRaw.slice(1, -1) : hostRaw; + if (allowPrivate) return { host }; + // IP literals are checked directly and never resolved. + if (isIP(host) !== 0) { + if (isBlockedAddress(host)) { + throw new Error(`Refusing to fetch private address: "${host}"`); + } + return { host }; + } + // Literal "localhost" / loopback aliases. if (host === 'localhost' || host.endsWith('.localhost')) { throw new Error(`Refusing to fetch private host: "${host}"`); } - if ( - host === '::1' || - host === '::' || - host.startsWith('fe80:') || - host.startsWith('fc') || - host.startsWith('fd') - ) { - throw new Error(`Refusing to fetch private host: "${host}"`); + // Hostnames must be resolved and every resulting address checked — a + // public-looking domain can point at loopback (e.g. localtest.me) or any + // internal address. The validated answers are returned so the caller can + // pin the connection to them (TOCTOU / DNS-rebinding protection). + let addresses: LookupAddress[]; + try { + addresses = await lookup(host, { all: true }); + } catch (error) { + const detail = error instanceof Error ? error.message : String(error); + throw new Error(`Cannot resolve host "${host}" for the fetch safety check: ${detail}`, { + cause: error, + }); } - const v4 = /^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/.exec(host); - if (v4 !== null) { - const octets = [v4[1], v4[2], v4[3], v4[4]].map(Number); - if (octets.some((n) => !Number.isInteger(n) || n < 0 || n > 255)) { - throw new Error(`Invalid IPv4 literal: "${host}"`); - } - const [a, b] = octets as [number, number, number, number]; - const isLoopback = a === 127; - const isPrivate10 = a === 10; - const isPrivate192 = a === 192 && b === 168; - const isPrivate172 = a === 172 && b >= 16 && b <= 31; - const isLinkLocal = a === 169 && b === 254; - const isZero = a === 0; - const isCgnat = a === 100 && b >= 64 && b <= 127; - if ( - isLoopback || - isPrivate10 || - isPrivate192 || - isPrivate172 || - isLinkLocal || - isZero || - isCgnat - ) { - throw new Error(`Refusing to fetch private address: "${host}"`); + for (const { address } of addresses) { + if (isBlockedAddress(address)) { + throw new Error(`Refusing to fetch host "${host}": resolves to private address "${address}".`); } } + return { host, addresses }; } + +/** + * Build a `net`/`tls` lookup hook that answers `host` from the validated + * address set, so the connect-time resolution cannot drift from what the + * safety check approved. Anything else is delegated to the real resolver + * (a per-hop Agent only ever connects to its own origin, but stay + * functional if reused elsewhere). + */ +function pinnedLookup(host: string, addresses: LookupAddress[]): LookupFunction { + return (hostname: string, options: LookupOptions | undefined, callback: PinnedLookupCallback) => { + if (hostname !== host) { + callbackLookup(hostname, options ?? {}, callback); + return; + } + if (options?.all === true) { + callback(null, [...addresses]); + return; + } + const single = addresses.find((entry) => entry.family === options?.family) ?? addresses[0]!; + callback(null, single.address, single.family); + }; +} + +type PinnedLookupCallback = ( + err: NodeJS.ErrnoException | null, + addressOrList: string | LookupAddress[], + family?: number, +) => void; diff --git a/packages/agent-core-v2/test/app/web/providers/local-fetch-url.test.ts b/packages/agent-core-v2/test/app/web/providers/local-fetch-url.test.ts new file mode 100644 index 0000000000..b098ea2d7e --- /dev/null +++ b/packages/agent-core-v2/test/app/web/providers/local-fetch-url.test.ts @@ -0,0 +1,304 @@ +/** + * `web` domain tests — `LocalFetchURLProvider` SSRF guard and redirects. + * + * Locks in that the provider rejects URLs whose IP literal or resolved + * address is private / loopback / link-local (including IPv4-mapped IPv6 + * forms), fails closed on DNS errors, and follows redirects manually with + * every hop re-validated. DNS is mocked so tests stay hermetic. + */ + +import { lookup } from 'node:dns/promises'; + +import { Agent } from 'undici'; +import { afterEach, beforeEach, describe, expect, it, vi, type Mock } from 'vitest'; + +import { LocalFetchURLProvider } from '#/app/web/providers/local-fetch-url'; + +vi.mock('node:dns/promises', () => ({ lookup: vi.fn() })); + +const lookupMock = lookup as unknown as Mock; + +// Keep DNS hermetic: every hostname resolves to a public address unless a +// test overrides it (mockReset clears per-test overrides first). +beforeEach(() => { + lookupMock.mockReset(); + lookupMock.mockResolvedValue([{ address: '93.184.216.34', family: 4 }]); + // Connection pinning is skipped when a proxy is configured — keep the + // environment free of proxy variables so tests stay hermetic anywhere. + for (const key of ['http_proxy', 'HTTP_PROXY', 'https_proxy', 'HTTPS_PROXY', 'all_proxy', 'ALL_PROXY']) { + vi.stubEnv(key, ''); + } +}); + +afterEach(() => { + vi.unstubAllEnvs(); +}); + +function htmlResponse(body: string, contentType: string): Response { + return new Response(body, { + status: 200, + headers: { 'content-type': contentType }, + }); +} + +describe('LocalFetchURLProvider SSRF guard', () => { + it('rejects a loopback IPv4 literal without fetching or resolving DNS', async () => { + const fetchImpl = vi.fn(); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await expect(provider.fetch('http://127.0.0.1:1337/')).rejects.toThrow( + 'Refusing to fetch private address', + ); + expect(fetchImpl).not.toHaveBeenCalled(); + expect(lookupMock).not.toHaveBeenCalled(); + }); + + it('rejects an IPv4-mapped IPv6 literal', async () => { + const fetchImpl = vi.fn(); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await expect(provider.fetch('http://[::ffff:127.0.0.1]/')).rejects.toThrow( + 'Refusing to fetch private address', + ); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + + it('rejects localhost and *.localhost aliases', async () => { + const fetchImpl = vi.fn(); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await expect(provider.fetch('http://localhost:1337/')).rejects.toThrow( + 'Refusing to fetch private host', + ); + await expect(provider.fetch('http://ev1l.localhost/')).rejects.toThrow( + 'Refusing to fetch private host', + ); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + + it('rejects a hostname that resolves to a loopback address', async () => { + lookupMock.mockResolvedValue([ + { address: '::1', family: 6 }, + { address: '127.0.0.1', family: 4 }, + ]); + const fetchImpl = vi.fn(); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await expect(provider.fetch('http://localtest.me/')).rejects.toThrow( + 'resolves to private address', + ); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + + it('rejects a hostname that resolves to an IPv4-mapped IPv6 address', async () => { + lookupMock.mockResolvedValue([{ address: '::ffff:169.254.169.254', family: 6 }]); + const fetchImpl = vi.fn(); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await expect(provider.fetch('http://sneaky.example.com/')).rejects.toThrow( + 'resolves to private address', + ); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + + it('fails closed when DNS resolution fails', async () => { + lookupMock.mockRejectedValue(new Error('getaddrinfo ENOTFOUND b0rked.example')); + const fetchImpl = vi.fn(); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await expect(provider.fetch('http://b0rked.example/')).rejects.toThrow('Cannot resolve host'); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + + it('rejects non-http(s) schemes before any network access', async () => { + const fetchImpl = vi.fn(); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await expect(provider.fetch('file:///etc/passwd')).rejects.toThrow('Unsupported URL scheme'); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + + it('fetches public hosts normally', async () => { + const fetchImpl = vi.fn().mockResolvedValue(htmlResponse('ok', 'text/plain')); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + const result = await provider.fetch('https://example.com/'); + + expect(result.content).toBe('ok'); + expect(fetchImpl).toHaveBeenCalledTimes(1); + expect(lookupMock).toHaveBeenCalledWith('example.com', { all: true }); + }); + + it('skips all checks when allowPrivateAddresses is set', async () => { + const fetchImpl = vi.fn().mockResolvedValue(htmlResponse('local', 'text/plain')); + const provider = new LocalFetchURLProvider({ fetchImpl, allowPrivateAddresses: true }); + + const result = await provider.fetch('http://127.0.0.1:1337/'); + + expect(result.content).toBe('local'); + expect(fetchImpl).toHaveBeenCalledTimes(1); + expect(lookupMock).not.toHaveBeenCalled(); + }); +}); + +describe('LocalFetchURLProvider redirects', () => { + it('follows a redirect after re-validating the target, fetching manually', async () => { + const fetchImpl = vi + .fn() + .mockResolvedValueOnce( + new Response(null, { + status: 302, + headers: { location: 'https://cdn.example.com/page' }, + }), + ) + .mockResolvedValueOnce(htmlResponse('final body', 'text/plain')); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + const result = await provider.fetch('https://example.com/start'); + + expect(result.content).toBe('final body'); + expect(fetchImpl).toHaveBeenCalledTimes(2); + expect(lookupMock).toHaveBeenCalledWith('cdn.example.com', { all: true }); + const [, firstInit] = fetchImpl.mock.calls[0]!; + expect((firstInit as RequestInit).redirect).toBe('manual'); + }); + + it('resolves relative redirect targets against the current URL', async () => { + const fetchImpl = vi + .fn() + .mockResolvedValueOnce(new Response(null, { status: 301, headers: { location: '/final' } })) + .mockResolvedValueOnce(htmlResponse('ok', 'text/plain')); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await provider.fetch('https://example.com/start'); + + const [secondUrl] = fetchImpl.mock.calls[1]!; + expect(secondUrl).toBe('https://example.com/final'); + }); + + it('refuses a redirect to a private IP literal', async () => { + const fetchImpl = vi.fn().mockResolvedValueOnce( + new Response(null, { + status: 302, + headers: { location: 'http://169.254.169.254/latest/meta-data' }, + }), + ); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await expect(provider.fetch('https://example.com/start')).rejects.toThrow( + 'Refusing to fetch private address', + ); + expect(fetchImpl).toHaveBeenCalledTimes(1); + }); + + it('refuses a redirect whose target host resolves to a private address', async () => { + lookupMock.mockImplementation(async (host: string) => + host === 'internal.example.com' + ? [{ address: '10.0.0.7', family: 4 }] + : [{ address: '93.184.216.34', family: 4 }], + ); + const fetchImpl = vi.fn().mockResolvedValueOnce( + new Response(null, { + status: 302, + headers: { location: 'https://internal.example.com/' }, + }), + ); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await expect(provider.fetch('https://example.com/')).rejects.toThrow( + 'resolves to private address', + ); + expect(fetchImpl).toHaveBeenCalledTimes(1); + }); + + it('gives up after too many redirects', async () => { + const fetchImpl = vi.fn().mockImplementation( + async () => new Response(null, { status: 302, headers: { location: '/loop' } }), + ); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await expect(provider.fetch('https://example.com/loop')).rejects.toThrow( + 'Too many redirects', + ); + expect(fetchImpl).toHaveBeenCalledTimes(11); + }); + + it('treats a redirect response without a Location header as final', async () => { + const fetchImpl = vi + .fn() + .mockResolvedValueOnce( + new Response('odd', { status: 302, headers: { 'content-type': 'text/plain' } }), + ); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + const result = await provider.fetch('https://example.com/odd'); + + expect(result.content).toBe('odd'); + expect(fetchImpl).toHaveBeenCalledTimes(1); + }); +}); + +describe('LocalFetchURLProvider connection pinning', () => { + it('pins a public-host fetch to the addresses validated by the safety check', async () => { + const fetchImpl = vi.fn().mockResolvedValue(htmlResponse('ok', 'text/plain')); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + const result = await provider.fetch('https://example.com/'); + + expect(result.content).toBe('ok'); + const [, init] = fetchImpl.mock.calls[0]!; + const dispatcher = (init as RequestInit).dispatcher; + expect(dispatcher).toBeInstanceOf(Agent); + // The DNS answer was validated once and reused for the connection. + expect(lookupMock).toHaveBeenCalledTimes(1); + // The per-hop Agent is closed once the body has been consumed. + expect((dispatcher as Agent).closed).toBe(true); + }); + + it('pins every redirect hop to its own validated addresses and closes both Agents', async () => { + const fetchImpl = vi + .fn() + .mockResolvedValueOnce(new Response(null, { status: 302, headers: { location: '/next' } })) + .mockResolvedValueOnce(htmlResponse('done', 'text/plain')); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await provider.fetch('https://example.com/start'); + + const first = (fetchImpl.mock.calls[0]![1] as RequestInit).dispatcher; + const second = (fetchImpl.mock.calls[1]![1] as RequestInit).dispatcher; + expect(first).toBeInstanceOf(Agent); + expect(second).toBeInstanceOf(Agent); + expect(first).not.toBe(second); + expect((first as Agent).closed).toBe(true); + expect((second as Agent).closed).toBe(true); + expect(lookupMock).toHaveBeenCalledTimes(2); + }); + + it('passes no dispatcher for an IP literal', async () => { + const fetchImpl = vi.fn().mockResolvedValue(htmlResponse('ok', 'text/plain')); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await provider.fetch('http://93.184.216.34/'); + + expect((fetchImpl.mock.calls[0]![1] as RequestInit).dispatcher).toBeUndefined(); + }); + + it('passes no dispatcher when allowPrivateAddresses is set', async () => { + const fetchImpl = vi.fn().mockResolvedValue(htmlResponse('ok', 'text/plain')); + const provider = new LocalFetchURLProvider({ fetchImpl, allowPrivateAddresses: true }); + + await provider.fetch('https://example.com/'); + + expect((fetchImpl.mock.calls[0]![1] as RequestInit).dispatcher).toBeUndefined(); + }); + + it('passes no dispatcher when an HTTP proxy is configured', async () => { + vi.stubEnv('http_proxy', 'http://proxy.example:8080'); + const fetchImpl = vi.fn().mockResolvedValue(htmlResponse('ok', 'text/plain')); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await provider.fetch('https://example.com/'); + + expect((fetchImpl.mock.calls[0]![1] as RequestInit).dispatcher).toBeUndefined(); + }); +}); diff --git a/packages/agent-core-v2/test/app/web/tools/fetch-url.test.ts b/packages/agent-core-v2/test/app/web/tools/fetch-url.test.ts index 2ad970a042..233833062d 100644 --- a/packages/agent-core-v2/test/app/web/tools/fetch-url.test.ts +++ b/packages/agent-core-v2/test/app/web/tools/fetch-url.test.ts @@ -7,13 +7,24 @@ * tool re-throws aborts so the executor can classify user cancellation. */ -import { describe, expect, it, vi } from 'vitest'; +import { lookup } from 'node:dns/promises'; + +import { beforeEach, describe, expect, it, vi, type Mock } from 'vitest'; import type { ExecutableToolContext, ExecutableToolResult, ToolExecution } from '#/tool/toolContract'; import { LocalFetchURLProvider } from '#/app/web/providers/local-fetch-url'; import { FetchURLTool } from '#/app/web/tools/fetch-url'; import type { UrlFetcher, UrlFetchResult } from '#/app/web/tools/fetch-url-types'; +vi.mock('node:dns/promises', () => ({ lookup: vi.fn() })); + +// LocalFetchURLProvider resolves hostnames before fetching; keep DNS +// hermetic so provider-level tests never touch the real resolver. +beforeEach(() => { + (lookup as unknown as Mock).mockReset(); + (lookup as unknown as Mock).mockResolvedValue([{ address: '93.184.216.34', family: 4 }]); +}); + function isPromiseLike(value: ToolExecution | Promise): value is Promise { return typeof (value as Promise).then === 'function'; } diff --git a/packages/agent-core/src/tools/providers/local-fetch-url.ts b/packages/agent-core/src/tools/providers/local-fetch-url.ts index af10a8ca31..eaf3704898 100644 --- a/packages/agent-core/src/tools/providers/local-fetch-url.ts +++ b/packages/agent-core/src/tools/providers/local-fetch-url.ts @@ -2,7 +2,10 @@ * LocalFetchURLProvider — host-side URL fetcher. * * Flow: - * 1. GET the URL with a Chrome-like UA. + * 1. Validate the URL against the SSRF rules (scheme, IP literals, DNS + * resolution) and GET it with a Chrome-like UA, following redirects + * manually with every hop re-validated and pinned to the validated + * addresses. * 2. Reject HTTP >= 400 with the status code in the message. * 3. Reject responses larger than `maxBytes` (content-length first, * then measured body length as a defensive second check). @@ -14,9 +17,15 @@ * before throwing a "meaningful content" error. */ +import { lookup as callbackLookup, type LookupAddress, type LookupOptions } from 'node:dns'; +import { lookup } from 'node:dns/promises'; +import { BlockList, isIP, type LookupFunction } from 'node:net'; + import { Readability } from '@mozilla/readability'; import { parseHTML as rawParseHTML } from 'linkedom'; +import { Agent, type Dispatcher } from 'undici'; +import { isProxyConfigured } from '../../utils/proxy'; import { HttpFetchError, type UrlFetcher, type UrlFetchResult } from '../builtin'; // Readability's .d.ts references the global `Document` type, but this @@ -43,6 +52,10 @@ const DEFAULT_USER_AGENT = const DEFAULT_MAX_BYTES = 10 * 1024 * 1024; +const MAX_REDIRECT_HOPS = 10; + +const REDIRECT_STATUSES = new Set([301, 302, 303, 307, 308]); + export interface LocalFetchURLProviderOptions { userAgent?: string; fetchImpl?: typeof fetch; @@ -58,15 +71,52 @@ export interface LocalFetchURLProviderOptions { } /** - * SSRF guard — reject non-http(s) schemes and (by default) any hostname - * that is, or parses as, a private / loopback / link-local / ULA IP - * literal. This is a *static* check against the URL string; it does NOT - * do DNS resolution, so a domain that resolves to a private IP via - * DNS-rebinding is **not** caught here. That attack is a known - * limitation; mitigations (e.g. pinning the resolved IP through to - * fetch) are left for a follow-up. + * SSRF blocklist: loopback / RFC 1918 / link-local / CGNAT / ULA and + * "this network", for both address families. BlockList.check() maps + * IPv4-mapped IPv6 addresses (e.g. ::ffff:127.0.0.1) onto the IPv4 + * subnets, so mapped literals cannot slip past the v4 rules. */ -function assertSafeFetchTarget(url: string, allowPrivate: boolean): void { +const PRIVATE_ADDRESS_BLOCKLIST = (() => { + const list = new BlockList(); + list.addSubnet('0.0.0.0', 8, 'ipv4'); // "this network" + list.addSubnet('10.0.0.0', 8, 'ipv4'); + list.addSubnet('100.64.0.0', 10, 'ipv4'); // CGNAT + list.addSubnet('127.0.0.0', 8, 'ipv4'); // loopback + list.addSubnet('169.254.0.0', 16, 'ipv4'); // link-local / cloud metadata + list.addSubnet('172.16.0.0', 12, 'ipv4'); + list.addSubnet('192.168.0.0', 16, 'ipv4'); + list.addSubnet('::', 128, 'ipv6'); // unspecified + list.addSubnet('::1', 128, 'ipv6'); // loopback + list.addSubnet('fc00::', 7, 'ipv6'); // ULA + list.addSubnet('fe80::', 10, 'ipv6'); // link-local + return list; +})(); + +function isBlockedAddress(address: string): boolean { + // Link-local addresses may carry a zone id ("fe80::1%en0") — strip it + // before matching. + const normalized = address.split('%', 1)[0] ?? address; + if (isIP(normalized) === 4) return PRIVATE_ADDRESS_BLOCKLIST.check(normalized, 'ipv4'); + return isIP(normalized) === 6 && PRIVATE_ADDRESS_BLOCKLIST.check(normalized, 'ipv6'); +} + +interface SafeFetchTarget { + /** Lowercased hostname with any IPv6 brackets stripped. */ + host: string; + /** Validated DNS answers to pin the connection to — absent when no lookup was needed. */ + addresses?: LookupAddress[]; +} + +/** + * SSRF guard — reject non-http(s) schemes and (by default) anything that + * resolves to a private / loopback / link-local / ULA address: IP literals + * are checked directly, hostnames are resolved via DNS and every resulting + * address is checked. Re-run for every redirect hop by the caller. Returns + * the validated DNS answers so the connection can be pinned to them — + * otherwise the connect-time re-resolution could be answered differently + * (TOCTOU / DNS rebinding). + */ +async function resolveSafeFetchTarget(url: string, allowPrivate: boolean): Promise { let parsed: URL; try { parsed = new URL(url); @@ -76,58 +126,70 @@ function assertSafeFetchTarget(url: string, allowPrivate: boolean): void { if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') { throw new Error(`Unsupported URL scheme "${parsed.protocol}" — only http(s) allowed.`); } - if (allowPrivate) return; // URL hostname preserves surrounding `[ ]` for IPv6 literals on some // Node versions (and not others). Strip them for uniform comparison. const hostRaw = parsed.hostname.toLowerCase(); const host = hostRaw.startsWith('[') && hostRaw.endsWith(']') ? hostRaw.slice(1, -1) : hostRaw; + if (allowPrivate) return { host }; + // IP literals are checked directly and never resolved. + if (isIP(host) !== 0) { + if (isBlockedAddress(host)) { + throw new Error(`Refusing to fetch private address: "${host}"`); + } + return { host }; + } // Literal "localhost" / loopback aliases. if (host === 'localhost' || host.endsWith('.localhost')) { throw new Error(`Refusing to fetch private host: "${host}"`); } - // IPv6 loopback / ULA / link-local. Check after bracket strip. - if ( - host === '::1' || - host === '::' || - host.startsWith('fe80:') || - host.startsWith('fc') || - host.startsWith('fd') - ) { - throw new Error(`Refusing to fetch private host: "${host}"`); + // Hostnames must be resolved and every resulting address checked — a + // public-looking domain can point at loopback (e.g. localtest.me) or any + // internal address. + let addresses: LookupAddress[]; + try { + addresses = await lookup(host, { all: true }); + } catch (error) { + const detail = error instanceof Error ? error.message : String(error); + throw new Error(`Cannot resolve host "${host}" for the fetch safety check: ${detail}`, { + cause: error, + }); } - // IPv4 literal — only check when the hostname is a dotted-quad; normal - // domains will never match. - const v4 = /^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/.exec(host); - if (v4 !== null) { - const octets = [v4[1], v4[2], v4[3], v4[4]].map(Number); - if (octets.some((n) => !Number.isInteger(n) || n < 0 || n > 255)) { - throw new Error(`Invalid IPv4 literal: "${host}"`); - } - const [a, b] = octets as [number, number, number, number]; - // 127.0.0.0/8 loopback, 10.0.0.0/8, 192.168.0.0/16, - // 172.16.0.0/12, 169.254.0.0/16 link-local / AWS metadata, - // 0.0.0.0/8 "this network", 100.64.0.0/10 CGNAT. - const isLoopback = a === 127; - const isPrivate10 = a === 10; - const isPrivate192 = a === 192 && b === 168; - const isPrivate172 = a === 172 && b >= 16 && b <= 31; - const isLinkLocal = a === 169 && b === 254; - const isZero = a === 0; - const isCgnat = a === 100 && b >= 64 && b <= 127; - if ( - isLoopback || - isPrivate10 || - isPrivate192 || - isPrivate172 || - isLinkLocal || - isZero || - isCgnat - ) { - throw new Error(`Refusing to fetch private address: "${host}"`); + for (const { address } of addresses) { + if (isBlockedAddress(address)) { + throw new Error(`Refusing to fetch host "${host}": resolves to private address "${address}".`); } } + return { host, addresses }; +} + +/** + * Build a `net`/`tls` lookup hook that answers `host` from the validated + * address set, so the connect-time resolution cannot drift from what the + * safety check approved. Anything else is delegated to the real resolver + * (a per-hop Agent only ever connects to its own origin, but stay + * functional if reused elsewhere). + */ +function pinnedLookup(host: string, addresses: LookupAddress[]): LookupFunction { + return (hostname: string, options: LookupOptions | undefined, callback: PinnedLookupCallback) => { + if (hostname !== host) { + callbackLookup(hostname, options ?? {}, callback); + return; + } + if (options?.all === true) { + callback(null, [...addresses]); + return; + } + const single = addresses.find((entry) => entry.family === options?.family) ?? addresses[0]!; + callback(null, single.address, single.family); + }; } +type PinnedLookupCallback = ( + err: NodeJS.ErrnoException | null, + addressOrList: string | LookupAddress[], + family?: number, +) => void; + export class LocalFetchURLProvider implements UrlFetcher { private readonly userAgent: string; private readonly fetchImpl: typeof fetch; @@ -142,13 +204,24 @@ export class LocalFetchURLProvider implements UrlFetcher { } async fetch(url: string, _options?: { toolCallId?: string }): Promise { - assertSafeFetchTarget(url, this.allowPrivateAddresses); - - const response = await this.fetchImpl(url, { - method: 'GET', - headers: { 'User-Agent': this.userAgent }, - }); + // Pinned Agents are created per redirect hop and closed once the final + // body is consumed, so keep-alive sockets never linger. + const dispatchers: Dispatcher[] = []; + try { + const response = await this.requestWithValidatedRedirects(url, dispatchers); + return await this.readResponse(response); + } finally { + await Promise.all( + dispatchers.map((dispatcher) => + dispatcher.close().catch(() => { + /* already closed */ + }), + ), + ); + } + } + private async readResponse(response: Response): Promise { if (response.status >= 400) { // Drain the unused body so undici can release the socket back to // the keep-alive pool instead of leaking it on error paths. @@ -190,6 +263,71 @@ export class LocalFetchURLProvider implements UrlFetcher { return { content: this.extractMainContent(body), kind: 'extracted' }; } + /** + * GET `url`, following redirects manually. Every hop re-runs the full + * SSRF check (IP-literal + DNS) before the request goes out — a public + * URL must not be able to bounce the fetcher at an internal address. + * Redirects without a `Location` header are treated as final responses. + */ + private async requestWithValidatedRedirects( + url: string, + dispatchers: Dispatcher[], + ): Promise { + let currentUrl = url; + let redirects = 0; + for (;;) { + const target = await resolveSafeFetchTarget(currentUrl, this.allowPrivateAddresses); + const response = await this.fetchImpl(currentUrl, { + method: 'GET', + headers: { 'User-Agent': this.userAgent }, + redirect: 'manual', + dispatcher: this.pinnedDispatcherFor(target, dispatchers), + }); + if (!REDIRECT_STATUSES.has(response.status)) return response; + const location = response.headers.get('location'); + if (location === null) return response; + // Drain the unused body so undici can release the socket back to + // the keep-alive pool instead of leaking it on redirect hops. + await response.body?.cancel().catch(() => { + /* already closed */ + }); + if (redirects >= MAX_REDIRECT_HOPS) { + throw new Error( + `Too many redirects while fetching "${url}" (limit ${String(MAX_REDIRECT_HOPS)}).`, + ); + } + redirects += 1; + currentUrl = new URL(location, currentUrl).toString(); + } + } + + /** + * Pin the connection to the addresses the safety check just validated. + * undici resolves the origin again when it connects, so without pinning + * an attacker-controlled DNS could answer the check with a public IP and + * the connect with an internal one (TOCTOU / DNS rebinding). + */ + private pinnedDispatcherFor( + target: SafeFetchTarget, + dispatchers: Dispatcher[], + ): RequestInit['dispatcher'] { + // IP literals (and allowPrivate mode) need no pin — there is no second + // resolution to race. + if (target.addresses === undefined) return undefined; + // With an HTTP/SOCKS proxy configured, origin resolution happens on the + // proxy side; a direct-connect pinned Agent would bypass the proxy + // entirely, so pinning only applies to direct connections. + if (isProxyConfigured(process.env)) return undefined; + const dispatcher = new Agent({ + connect: { lookup: pinnedLookup(target.host, target.addresses) }, + }); + dispatchers.push(dispatcher); + // Compatible at runtime (undici is undici); the two type declarations — + // the package's own and the copy bundled with @types/node's global + // fetch — just can't see each other. + return dispatcher as unknown as RequestInit['dispatcher']; + } + private extractMainContent(html: string): string { // Readability mutates the DOM it parses, so parse twice — once for // the primary extractor and once for the fallback path. diff --git a/packages/agent-core/test/tools/providers/local-fetch-url.test.ts b/packages/agent-core/test/tools/providers/local-fetch-url.test.ts index 2c0ce931f1..4f6db65b4b 100644 --- a/packages/agent-core/test/tools/providers/local-fetch-url.test.ts +++ b/packages/agent-core/test/tools/providers/local-fetch-url.test.ts @@ -1,15 +1,41 @@ /** - * Covers: LocalFetchURLProvider content-kind reporting. + * Covers: LocalFetchURLProvider content-kind reporting, SSRF guard, and + * redirect handling. * * Verifies the provider tells callers whether the returned content is a * verbatim passthrough of the response body or the main text extracted - * from an HTML page. + * from an HTML page; that it rejects URLs whose IP literal or resolved + * address is private / loopback / link-local; and that redirects are + * followed manually with every hop re-validated. */ -import { describe, expect, it, vi } from 'vitest'; +import { lookup } from 'node:dns/promises'; + +import { Agent } from 'undici'; +import { afterEach, beforeEach, describe, expect, it, vi, type Mock } from 'vitest'; import { LocalFetchURLProvider } from '../../../src/tools/providers/local-fetch-url'; +vi.mock('node:dns/promises', () => ({ lookup: vi.fn() })); + +const lookupMock = lookup as unknown as Mock; + +// Keep DNS hermetic: every hostname resolves to a public address unless a +// test overrides it (mockReset clears per-test overrides first). +beforeEach(() => { + lookupMock.mockReset(); + lookupMock.mockResolvedValue([{ address: '93.184.216.34', family: 4 }]); + // Connection pinning is skipped when a proxy is configured — keep the + // environment free of proxy variables so tests stay hermetic anywhere. + for (const key of ['http_proxy', 'HTTP_PROXY', 'https_proxy', 'HTTPS_PROXY', 'all_proxy', 'ALL_PROXY']) { + vi.stubEnv(key, ''); + } +}); + +afterEach(() => { + vi.unstubAllEnvs(); +}); + function htmlResponse(body: string, contentType: string): Response { return new Response(body, { status: 200, @@ -56,3 +82,265 @@ describe('LocalFetchURLProvider content kind', () => { expect(result.content).toContain('quick brown fox'); }); }); + +describe('LocalFetchURLProvider SSRF guard', () => { + it('rejects a loopback IPv4 literal without fetching or resolving DNS', async () => { + const fetchImpl = vi.fn(); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await expect(provider.fetch('http://127.0.0.1:1337/')).rejects.toThrow( + 'Refusing to fetch private address', + ); + expect(fetchImpl).not.toHaveBeenCalled(); + expect(lookupMock).not.toHaveBeenCalled(); + }); + + it('rejects an IPv4-mapped IPv6 literal', async () => { + const fetchImpl = vi.fn(); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await expect(provider.fetch('http://[::ffff:127.0.0.1]/')).rejects.toThrow( + 'Refusing to fetch private address', + ); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + + it('rejects localhost and *.localhost aliases', async () => { + const fetchImpl = vi.fn(); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await expect(provider.fetch('http://localhost:1337/')).rejects.toThrow( + 'Refusing to fetch private host', + ); + await expect(provider.fetch('http://ev1l.localhost/')).rejects.toThrow( + 'Refusing to fetch private host', + ); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + + it('rejects a hostname that resolves to a loopback address', async () => { + lookupMock.mockResolvedValue([ + { address: '::1', family: 6 }, + { address: '127.0.0.1', family: 4 }, + ]); + const fetchImpl = vi.fn(); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await expect(provider.fetch('http://localtest.me/')).rejects.toThrow( + 'resolves to private address', + ); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + + it('rejects a hostname that resolves to an IPv4-mapped IPv6 address', async () => { + lookupMock.mockResolvedValue([{ address: '::ffff:169.254.169.254', family: 6 }]); + const fetchImpl = vi.fn(); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await expect(provider.fetch('http://sneaky.example.com/')).rejects.toThrow( + 'resolves to private address', + ); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + + it('fails closed when DNS resolution fails', async () => { + lookupMock.mockRejectedValue(new Error('getaddrinfo ENOTFOUND b0rked.example')); + const fetchImpl = vi.fn(); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await expect(provider.fetch('http://b0rked.example/')).rejects.toThrow('Cannot resolve host'); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + + it('rejects non-http(s) schemes before any network access', async () => { + const fetchImpl = vi.fn(); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await expect(provider.fetch('file:///etc/passwd')).rejects.toThrow('Unsupported URL scheme'); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + + it('fetches public hosts normally', async () => { + const fetchImpl = vi.fn().mockResolvedValue(htmlResponse('ok', 'text/plain')); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + const result = await provider.fetch('https://example.com/'); + + expect(result.content).toBe('ok'); + expect(fetchImpl).toHaveBeenCalledTimes(1); + expect(lookupMock).toHaveBeenCalledWith('example.com', { all: true }); + }); + + it('skips all checks when allowPrivateAddresses is set', async () => { + const fetchImpl = vi.fn().mockResolvedValue(htmlResponse('local', 'text/plain')); + const provider = new LocalFetchURLProvider({ fetchImpl, allowPrivateAddresses: true }); + + const result = await provider.fetch('http://127.0.0.1:1337/'); + + expect(result.content).toBe('local'); + expect(fetchImpl).toHaveBeenCalledTimes(1); + expect(lookupMock).not.toHaveBeenCalled(); + }); +}); + +describe('LocalFetchURLProvider redirects', () => { + it('follows a redirect after re-validating the target, fetching manually', async () => { + const fetchImpl = vi + .fn() + .mockResolvedValueOnce( + new Response(null, { + status: 302, + headers: { location: 'https://cdn.example.com/page' }, + }), + ) + .mockResolvedValueOnce(htmlResponse('final body', 'text/plain')); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + const result = await provider.fetch('https://example.com/start'); + + expect(result.content).toBe('final body'); + expect(fetchImpl).toHaveBeenCalledTimes(2); + expect(lookupMock).toHaveBeenCalledWith('cdn.example.com', { all: true }); + const [, firstInit] = fetchImpl.mock.calls[0]!; + expect((firstInit as RequestInit).redirect).toBe('manual'); + }); + + it('resolves relative redirect targets against the current URL', async () => { + const fetchImpl = vi + .fn() + .mockResolvedValueOnce(new Response(null, { status: 301, headers: { location: '/final' } })) + .mockResolvedValueOnce(htmlResponse('ok', 'text/plain')); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await provider.fetch('https://example.com/start'); + + const [secondUrl] = fetchImpl.mock.calls[1]!; + expect(secondUrl).toBe('https://example.com/final'); + }); + + it('refuses a redirect to a private IP literal', async () => { + const fetchImpl = vi.fn().mockResolvedValueOnce( + new Response(null, { + status: 302, + headers: { location: 'http://169.254.169.254/latest/meta-data' }, + }), + ); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await expect(provider.fetch('https://example.com/start')).rejects.toThrow( + 'Refusing to fetch private address', + ); + expect(fetchImpl).toHaveBeenCalledTimes(1); + }); + + it('refuses a redirect whose target host resolves to a private address', async () => { + lookupMock.mockImplementation(async (host: string) => + host === 'internal.example.com' + ? [{ address: '10.0.0.7', family: 4 }] + : [{ address: '93.184.216.34', family: 4 }], + ); + const fetchImpl = vi.fn().mockResolvedValueOnce( + new Response(null, { + status: 302, + headers: { location: 'https://internal.example.com/' }, + }), + ); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await expect(provider.fetch('https://example.com/')).rejects.toThrow( + 'resolves to private address', + ); + expect(fetchImpl).toHaveBeenCalledTimes(1); + }); + + it('gives up after too many redirects', async () => { + const fetchImpl = vi.fn().mockImplementation( + async () => new Response(null, { status: 302, headers: { location: '/loop' } }), + ); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await expect(provider.fetch('https://example.com/loop')).rejects.toThrow( + 'Too many redirects', + ); + expect(fetchImpl).toHaveBeenCalledTimes(11); + }); + + it('treats a redirect response without a Location header as final', async () => { + const fetchImpl = vi + .fn() + .mockResolvedValueOnce( + new Response('odd', { status: 302, headers: { 'content-type': 'text/plain' } }), + ); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + const result = await provider.fetch('https://example.com/odd'); + + expect(result.content).toBe('odd'); + expect(fetchImpl).toHaveBeenCalledTimes(1); + }); +}); + +describe('LocalFetchURLProvider connection pinning', () => { + it('pins a public-host fetch to the addresses validated by the safety check', async () => { + const fetchImpl = vi.fn().mockResolvedValue(htmlResponse('ok', 'text/plain')); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + const result = await provider.fetch('https://example.com/'); + + expect(result.content).toBe('ok'); + const [, init] = fetchImpl.mock.calls[0]!; + const dispatcher = (init as RequestInit).dispatcher; + expect(dispatcher).toBeInstanceOf(Agent); + // The DNS answer was validated once and reused for the connection. + expect(lookupMock).toHaveBeenCalledTimes(1); + // The per-hop Agent is closed once the body has been consumed. + expect((dispatcher as Agent).closed).toBe(true); + }); + + it('pins every redirect hop to its own validated addresses and closes both Agents', async () => { + const fetchImpl = vi + .fn() + .mockResolvedValueOnce(new Response(null, { status: 302, headers: { location: '/next' } })) + .mockResolvedValueOnce(htmlResponse('done', 'text/plain')); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await provider.fetch('https://example.com/start'); + + const first = (fetchImpl.mock.calls[0]![1] as RequestInit).dispatcher; + const second = (fetchImpl.mock.calls[1]![1] as RequestInit).dispatcher; + expect(first).toBeInstanceOf(Agent); + expect(second).toBeInstanceOf(Agent); + expect(first).not.toBe(second); + expect((first as Agent).closed).toBe(true); + expect((second as Agent).closed).toBe(true); + expect(lookupMock).toHaveBeenCalledTimes(2); + }); + + it('passes no dispatcher for an IP literal', async () => { + const fetchImpl = vi.fn().mockResolvedValue(htmlResponse('ok', 'text/plain')); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await provider.fetch('http://93.184.216.34/'); + + expect((fetchImpl.mock.calls[0]![1] as RequestInit).dispatcher).toBeUndefined(); + }); + + it('passes no dispatcher when allowPrivateAddresses is set', async () => { + const fetchImpl = vi.fn().mockResolvedValue(htmlResponse('ok', 'text/plain')); + const provider = new LocalFetchURLProvider({ fetchImpl, allowPrivateAddresses: true }); + + await provider.fetch('https://example.com/'); + + expect((fetchImpl.mock.calls[0]![1] as RequestInit).dispatcher).toBeUndefined(); + }); + + it('passes no dispatcher when an HTTP proxy is configured', async () => { + vi.stubEnv('http_proxy', 'http://proxy.example:8080'); + const fetchImpl = vi.fn().mockResolvedValue(htmlResponse('ok', 'text/plain')); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await provider.fetch('https://example.com/'); + + expect((fetchImpl.mock.calls[0]![1] as RequestInit).dispatcher).toBeUndefined(); + }); +}); From 86a1b7851ecc84056c3d6715131f699637a6d09d Mon Sep 17 00:00:00 2001 From: "haozhe.yang" Date: Thu, 16 Jul 2026 22:52:50 +0800 Subject: [PATCH 2/5] chore: add changeset for FetchURL SSRF hardening --- .changeset/secure-fetch-url-ssrf.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/secure-fetch-url-ssrf.md diff --git a/.changeset/secure-fetch-url-ssrf.md b/.changeset/secure-fetch-url-ssrf.md new file mode 100644 index 0000000000..eba3c7622f --- /dev/null +++ b/.changeset/secure-fetch-url-ssrf.md @@ -0,0 +1,5 @@ +--- +"@moonshot-ai/kimi-code": patch +--- + +Fix the built-in URL fetch tool's network safeguards: crafted domains and redirect chains can no longer reach loopback or internal network services. From 4ddbc88a8d897d13edbe3639d8d1d202a0665ba0 Mon Sep 17 00:00:00 2001 From: "haozhe.yang" Date: Thu, 16 Jul 2026 22:59:57 +0800 Subject: [PATCH 3/5] test: bridge undici type declarations in fetch pinning tests --- .../test/app/web/providers/local-fetch-url.test.ts | 13 ++++++++++--- .../test/tools/providers/local-fetch-url.test.ts | 13 ++++++++++--- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/packages/agent-core-v2/test/app/web/providers/local-fetch-url.test.ts b/packages/agent-core-v2/test/app/web/providers/local-fetch-url.test.ts index b098ea2d7e..6362ff6f39 100644 --- a/packages/agent-core-v2/test/app/web/providers/local-fetch-url.test.ts +++ b/packages/agent-core-v2/test/app/web/providers/local-fetch-url.test.ts @@ -18,6 +18,13 @@ vi.mock('node:dns/promises', () => ({ lookup: vi.fn() })); const lookupMock = lookup as unknown as Mock; +// The init's dispatcher property is typed by @types/node's bundled +// undici-types, while the runtime value is the undici package's Agent — +// convert through unknown to bridge the two declarations. +function asUndiciAgent(dispatcher: RequestInit['dispatcher']): Agent { + return dispatcher as unknown as Agent; +} + // Keep DNS hermetic: every hostname resolves to a public address unless a // test overrides it (mockReset clears per-test overrides first). beforeEach(() => { @@ -252,7 +259,7 @@ describe('LocalFetchURLProvider connection pinning', () => { // The DNS answer was validated once and reused for the connection. expect(lookupMock).toHaveBeenCalledTimes(1); // The per-hop Agent is closed once the body has been consumed. - expect((dispatcher as Agent).closed).toBe(true); + expect(asUndiciAgent(dispatcher).closed).toBe(true); }); it('pins every redirect hop to its own validated addresses and closes both Agents', async () => { @@ -269,8 +276,8 @@ describe('LocalFetchURLProvider connection pinning', () => { expect(first).toBeInstanceOf(Agent); expect(second).toBeInstanceOf(Agent); expect(first).not.toBe(second); - expect((first as Agent).closed).toBe(true); - expect((second as Agent).closed).toBe(true); + expect(asUndiciAgent(first).closed).toBe(true); + expect(asUndiciAgent(second).closed).toBe(true); expect(lookupMock).toHaveBeenCalledTimes(2); }); diff --git a/packages/agent-core/test/tools/providers/local-fetch-url.test.ts b/packages/agent-core/test/tools/providers/local-fetch-url.test.ts index 4f6db65b4b..c1001db9c9 100644 --- a/packages/agent-core/test/tools/providers/local-fetch-url.test.ts +++ b/packages/agent-core/test/tools/providers/local-fetch-url.test.ts @@ -20,6 +20,13 @@ vi.mock('node:dns/promises', () => ({ lookup: vi.fn() })); const lookupMock = lookup as unknown as Mock; +// The init's dispatcher property is typed by @types/node's bundled +// undici-types, while the runtime value is the undici package's Agent — +// convert through unknown to bridge the two declarations. +function asUndiciAgent(dispatcher: RequestInit['dispatcher']): Agent { + return dispatcher as unknown as Agent; +} + // Keep DNS hermetic: every hostname resolves to a public address unless a // test overrides it (mockReset clears per-test overrides first). beforeEach(() => { @@ -294,7 +301,7 @@ describe('LocalFetchURLProvider connection pinning', () => { // The DNS answer was validated once and reused for the connection. expect(lookupMock).toHaveBeenCalledTimes(1); // The per-hop Agent is closed once the body has been consumed. - expect((dispatcher as Agent).closed).toBe(true); + expect(asUndiciAgent(dispatcher).closed).toBe(true); }); it('pins every redirect hop to its own validated addresses and closes both Agents', async () => { @@ -311,8 +318,8 @@ describe('LocalFetchURLProvider connection pinning', () => { expect(first).toBeInstanceOf(Agent); expect(second).toBeInstanceOf(Agent); expect(first).not.toBe(second); - expect((first as Agent).closed).toBe(true); - expect((second as Agent).closed).toBe(true); + expect(asUndiciAgent(first).closed).toBe(true); + expect(asUndiciAgent(second).closed).toBe(true); expect(lookupMock).toHaveBeenCalledTimes(2); }); From c729bebfcfbfca5c12cc89c707c8306c25113c2c Mon Sep 17 00:00:00 2001 From: "haozhe.yang" Date: Thu, 16 Jul 2026 23:05:49 +0800 Subject: [PATCH 4/5] fix(security): keep dispatcher option lib-agnostic for DOM typecheck consumers --- .../src/app/web/providers/local-fetch-url.ts | 14 +++++++------- .../src/tools/providers/local-fetch-url.ts | 14 +++++++------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/packages/agent-core-v2/src/app/web/providers/local-fetch-url.ts b/packages/agent-core-v2/src/app/web/providers/local-fetch-url.ts index fa37f72762..6b6aa1c72e 100644 --- a/packages/agent-core-v2/src/app/web/providers/local-fetch-url.ts +++ b/packages/agent-core-v2/src/app/web/providers/local-fetch-url.ts @@ -132,8 +132,11 @@ export class LocalFetchURLProvider implements UrlFetcher { headers: { 'User-Agent': this.userAgent }, signal, redirect: 'manual', - dispatcher: this.pinnedDispatcherFor(target, dispatchers), - }); + // `dispatcher` is honored by undici at runtime but absent from + // DOM's RequestInit type (DOM-lib consumers typecheck this source) + // — hide it behind `unknown` to stay lib-agnostic. + dispatcher: this.pinnedDispatcherFor(target, dispatchers) as unknown, + } as RequestInit); if (!REDIRECT_STATUSES.has(response.status)) return response; const location = response.headers.get('location'); if (location === null) return response; @@ -158,7 +161,7 @@ export class LocalFetchURLProvider implements UrlFetcher { private pinnedDispatcherFor( target: SafeFetchTarget, dispatchers: Dispatcher[], - ): RequestInit['dispatcher'] { + ): Dispatcher | undefined { // IP literals (and allowPrivate mode) need no pin — there is no second // resolution to race. if (target.addresses === undefined) return undefined; @@ -170,10 +173,7 @@ export class LocalFetchURLProvider implements UrlFetcher { connect: { lookup: pinnedLookup(target.host, target.addresses) }, }); dispatchers.push(dispatcher); - // Compatible at runtime (undici is undici); the two type declarations — - // the package's own and the copy bundled with @types/node's global - // fetch — just can't see each other. - return dispatcher as unknown as RequestInit['dispatcher']; + return dispatcher; } private extractMainContent(html: string): string { diff --git a/packages/agent-core/src/tools/providers/local-fetch-url.ts b/packages/agent-core/src/tools/providers/local-fetch-url.ts index eaf3704898..ffd24223c4 100644 --- a/packages/agent-core/src/tools/providers/local-fetch-url.ts +++ b/packages/agent-core/src/tools/providers/local-fetch-url.ts @@ -281,8 +281,11 @@ export class LocalFetchURLProvider implements UrlFetcher { method: 'GET', headers: { 'User-Agent': this.userAgent }, redirect: 'manual', - dispatcher: this.pinnedDispatcherFor(target, dispatchers), - }); + // `dispatcher` is honored by undici at runtime but absent from + // DOM's RequestInit type (DOM-lib consumers typecheck this source) + // — hide it behind `unknown` to stay lib-agnostic. + dispatcher: this.pinnedDispatcherFor(target, dispatchers) as unknown, + } as RequestInit); if (!REDIRECT_STATUSES.has(response.status)) return response; const location = response.headers.get('location'); if (location === null) return response; @@ -310,7 +313,7 @@ export class LocalFetchURLProvider implements UrlFetcher { private pinnedDispatcherFor( target: SafeFetchTarget, dispatchers: Dispatcher[], - ): RequestInit['dispatcher'] { + ): Dispatcher | undefined { // IP literals (and allowPrivate mode) need no pin — there is no second // resolution to race. if (target.addresses === undefined) return undefined; @@ -322,10 +325,7 @@ export class LocalFetchURLProvider implements UrlFetcher { connect: { lookup: pinnedLookup(target.host, target.addresses) }, }); dispatchers.push(dispatcher); - // Compatible at runtime (undici is undici); the two type declarations — - // the package's own and the copy bundled with @types/node's global - // fetch — just can't see each other. - return dispatcher as unknown as RequestInit['dispatcher']; + return dispatcher; } private extractMainContent(html: string): string { From 4ecf1c3f3f688d93eb45c477cb641818e5c50dbb Mon Sep 17 00:00:00 2001 From: "haozhe.yang" Date: Fri, 17 Jul 2026 11:06:45 +0800 Subject: [PATCH 5/5] =?UTF-8?q?fix(security):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20pin=20NO=5FPROXY=20bypasses,=20drain=20oversized=20?= =?UTF-8?q?bodies,=20header-only=20comments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../src/app/web/providers/local-fetch-url.ts | 94 +++++++------------ .../app/web/providers/local-fetch-url.test.ts | 42 +++++++++ .../src/tools/providers/local-fetch-url.ts | 30 ++++-- .../tools/providers/local-fetch-url.test.ts | 42 +++++++++ 4 files changed, 142 insertions(+), 66 deletions(-) diff --git a/packages/agent-core-v2/src/app/web/providers/local-fetch-url.ts b/packages/agent-core-v2/src/app/web/providers/local-fetch-url.ts index 6b6aa1c72e..bd785f0d21 100644 --- a/packages/agent-core-v2/src/app/web/providers/local-fetch-url.ts +++ b/packages/agent-core-v2/src/app/web/providers/local-fetch-url.ts @@ -1,3 +1,17 @@ +/** + * `web` domain (L4) — local `UrlFetcher` used when no managed fetch service + * is configured. GETs URLs with a Chrome-like UA and SSRF hardening: http(s) + * schemes only; unless `allowPrivateAddresses` is set, IP literals and + * DNS-resolved addresses in loopback / RFC1918 / link-local / CGNAT / ULA + * ranges are refused, including IPv4-mapped IPv6 forms; redirects are + * followed manually with the same validation re-run on every hop; and each + * request's connection is pinned to the DNS answers validation approved, so + * a connect-time re-resolution cannot be rebound elsewhere (pinning is + * skipped for IP literals and for requests a proxy will carry — NO_PROXY + * bypasses still pin). Oversized bodies are refused; plain texts pass + * through verbatim and HTML is reduced to its main text. + */ + import { lookup as callbackLookup, type LookupAddress, type LookupOptions } from 'node:dns'; import { lookup } from 'node:dns/promises'; import { BlockList, isIP, type LookupFunction } from 'node:net'; @@ -6,7 +20,7 @@ import { Readability } from '@mozilla/readability'; import { parseHTML as rawParseHTML } from 'linkedom'; import { Agent, type Dispatcher } from 'undici'; -import { isProxyConfigured } from '#/_base/utils/proxy'; +import { isProxyConfigured, makeNoProxyMatcher, resolveNoProxy } from '#/_base/utils/proxy'; import { HttpFetchError, type UrlFetcher, type UrlFetchResult } from '../tools/fetch-url-types'; @@ -55,8 +69,6 @@ export class LocalFetchURLProvider implements UrlFetcher { url: string, options?: { toolCallId?: string; signal?: AbortSignal }, ): Promise { - // Pinned Agents are created per redirect hop and closed once the final - // body is consumed, so keep-alive sockets never linger. const dispatchers: Dispatcher[] = []; try { const response = await this.requestWithValidatedRedirects( @@ -89,6 +101,8 @@ export class LocalFetchURLProvider implements UrlFetcher { if (contentLengthRaw !== null) { const cl = Number(contentLengthRaw); if (Number.isFinite(cl) && cl > this.maxBytes) { + await response.body?.cancel().catch(() => { + }); throw new Error( `Response body too large: ${String(cl)} bytes exceeds maxBytes (${String(this.maxBytes)}).`, ); @@ -112,12 +126,6 @@ export class LocalFetchURLProvider implements UrlFetcher { return { content: this.extractMainContent(body), kind: 'extracted' }; } - /** - * GET `url`, following redirects manually. Every hop re-runs the full - * SSRF check (IP-literal + DNS) before the request goes out — a public - * URL must not be able to bounce the fetcher at an internal address. - * Redirects without a `Location` header are treated as final responses. - */ private async requestWithValidatedRedirects( url: string, signal: AbortSignal | undefined, @@ -132,9 +140,6 @@ export class LocalFetchURLProvider implements UrlFetcher { headers: { 'User-Agent': this.userAgent }, signal, redirect: 'manual', - // `dispatcher` is honored by undici at runtime but absent from - // DOM's RequestInit type (DOM-lib consumers typecheck this source) - // — hide it behind `unknown` to stay lib-agnostic. dispatcher: this.pinnedDispatcherFor(target, dispatchers) as unknown, } as RequestInit); if (!REDIRECT_STATUSES.has(response.status)) return response; @@ -152,23 +157,17 @@ export class LocalFetchURLProvider implements UrlFetcher { } } - /** - * Pin the connection to the addresses the safety check just validated. - * undici resolves the origin again when it connects, so without pinning - * an attacker-controlled DNS could answer the check with a public IP and - * the connect with an internal one (TOCTOU / DNS rebinding). - */ private pinnedDispatcherFor( target: SafeFetchTarget, dispatchers: Dispatcher[], ): Dispatcher | undefined { - // IP literals (and allowPrivate mode) need no pin — there is no second - // resolution to race. if (target.addresses === undefined) return undefined; - // With an HTTP/SOCKS proxy configured, origin resolution happens on the - // proxy side; a direct-connect pinned Agent would bypass the proxy - // entirely, so pinning only applies to direct connections. - if (isProxyConfigured(process.env)) return undefined; + if ( + isProxyConfigured(process.env) && + !makeNoProxyMatcher(resolveNoProxy(process.env))(target.host, target.port) + ) { + return undefined; + } const dispatcher = new Agent({ connect: { lookup: pinnedLookup(target.host, target.addresses) }, }); @@ -211,38 +210,31 @@ export class LocalFetchURLProvider implements UrlFetcher { } } -// SSRF blocklist: loopback / RFC 1918 / link-local / CGNAT / ULA and "this -// network", for both address families. BlockList.check() maps IPv4-mapped -// IPv6 addresses (e.g. ::ffff:127.0.0.1) onto the IPv4 subnets, so mapped -// literals cannot slip past the v4 rules. const PRIVATE_ADDRESS_BLOCKLIST = (() => { const list = new BlockList(); - list.addSubnet('0.0.0.0', 8, 'ipv4'); // "this network" + list.addSubnet('0.0.0.0', 8, 'ipv4'); list.addSubnet('10.0.0.0', 8, 'ipv4'); - list.addSubnet('100.64.0.0', 10, 'ipv4'); // CGNAT - list.addSubnet('127.0.0.0', 8, 'ipv4'); // loopback - list.addSubnet('169.254.0.0', 16, 'ipv4'); // link-local / cloud metadata + list.addSubnet('100.64.0.0', 10, 'ipv4'); + list.addSubnet('127.0.0.0', 8, 'ipv4'); + list.addSubnet('169.254.0.0', 16, 'ipv4'); list.addSubnet('172.16.0.0', 12, 'ipv4'); list.addSubnet('192.168.0.0', 16, 'ipv4'); - list.addSubnet('::', 128, 'ipv6'); // unspecified - list.addSubnet('::1', 128, 'ipv6'); // loopback - list.addSubnet('fc00::', 7, 'ipv6'); // ULA - list.addSubnet('fe80::', 10, 'ipv6'); // link-local + list.addSubnet('::', 128, 'ipv6'); + list.addSubnet('::1', 128, 'ipv6'); + list.addSubnet('fc00::', 7, 'ipv6'); + list.addSubnet('fe80::', 10, 'ipv6'); return list; })(); function isBlockedAddress(address: string): boolean { - // Link-local addresses may carry a zone id ("fe80::1%en0") — strip it - // before matching. const normalized = address.split('%', 1)[0] ?? address; if (isIP(normalized) === 4) return PRIVATE_ADDRESS_BLOCKLIST.check(normalized, 'ipv4'); return isIP(normalized) === 6 && PRIVATE_ADDRESS_BLOCKLIST.check(normalized, 'ipv6'); } interface SafeFetchTarget { - /** Lowercased hostname with any IPv6 brackets stripped. */ host: string; - /** Validated DNS answers to pin the connection to — absent when no lookup was needed. */ + port: string; addresses?: LookupAddress[]; } @@ -256,26 +248,19 @@ async function resolveSafeFetchTarget(url: string, allowPrivate: boolean): Promi if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') { throw new Error(`Unsupported URL scheme "${parsed.protocol}" — only http(s) allowed.`); } - // URL hostname preserves surrounding `[ ]` for IPv6 literals on some - // Node versions (and not others). Strip them for uniform comparison. const hostRaw = parsed.hostname.toLowerCase(); const host = hostRaw.startsWith('[') && hostRaw.endsWith(']') ? hostRaw.slice(1, -1) : hostRaw; - if (allowPrivate) return { host }; - // IP literals are checked directly and never resolved. + const port = parsed.port !== '' ? parsed.port : parsed.protocol === 'https:' ? '443' : '80'; + if (allowPrivate) return { host, port }; if (isIP(host) !== 0) { if (isBlockedAddress(host)) { throw new Error(`Refusing to fetch private address: "${host}"`); } - return { host }; + return { host, port }; } - // Literal "localhost" / loopback aliases. if (host === 'localhost' || host.endsWith('.localhost')) { throw new Error(`Refusing to fetch private host: "${host}"`); } - // Hostnames must be resolved and every resulting address checked — a - // public-looking domain can point at loopback (e.g. localtest.me) or any - // internal address. The validated answers are returned so the caller can - // pin the connection to them (TOCTOU / DNS-rebinding protection). let addresses: LookupAddress[]; try { addresses = await lookup(host, { all: true }); @@ -290,16 +275,9 @@ async function resolveSafeFetchTarget(url: string, allowPrivate: boolean): Promi throw new Error(`Refusing to fetch host "${host}": resolves to private address "${address}".`); } } - return { host, addresses }; + return { host, port, addresses }; } -/** - * Build a `net`/`tls` lookup hook that answers `host` from the validated - * address set, so the connect-time resolution cannot drift from what the - * safety check approved. Anything else is delegated to the real resolver - * (a per-hop Agent only ever connects to its own origin, but stay - * functional if reused elsewhere). - */ function pinnedLookup(host: string, addresses: LookupAddress[]): LookupFunction { return (hostname: string, options: LookupOptions | undefined, callback: PinnedLookupCallback) => { if (hostname !== host) { diff --git a/packages/agent-core-v2/test/app/web/providers/local-fetch-url.test.ts b/packages/agent-core-v2/test/app/web/providers/local-fetch-url.test.ts index 6362ff6f39..07673a727d 100644 --- a/packages/agent-core-v2/test/app/web/providers/local-fetch-url.test.ts +++ b/packages/agent-core-v2/test/app/web/providers/local-fetch-url.test.ts @@ -308,4 +308,46 @@ describe('LocalFetchURLProvider connection pinning', () => { expect((fetchImpl.mock.calls[0]![1] as RequestInit).dispatcher).toBeUndefined(); }); + + it('still pins when the request bypasses the proxy via NO_PROXY wildcard', async () => { + vi.stubEnv('http_proxy', 'http://proxy.example:8080'); + vi.stubEnv('no_proxy', '*'); + const fetchImpl = vi.fn().mockResolvedValue(htmlResponse('ok', 'text/plain')); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await provider.fetch('https://example.com/'); + + expect((fetchImpl.mock.calls[0]![1] as RequestInit).dispatcher).toBeInstanceOf(Agent); + }); + + it('still pins when NO_PROXY exempts the target host specifically', async () => { + vi.stubEnv('http_proxy', 'http://proxy.example:8080'); + vi.stubEnv('no_proxy', 'example.com'); + const fetchImpl = vi.fn().mockResolvedValue(htmlResponse('ok', 'text/plain')); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await provider.fetch('https://example.com/'); + + expect((fetchImpl.mock.calls[0]![1] as RequestInit).dispatcher).toBeInstanceOf(Agent); + }); + + it('rejects oversized responses by content-length and still closes the pinned Agent', async () => { + const fetchImpl = vi.fn().mockResolvedValue( + new Response('short', { + status: 200, + headers: { + 'content-type': 'text/plain', + 'content-length': String(11 * 1024 * 1024), + }, + }), + ); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await expect(provider.fetch('https://example.com/big')).rejects.toThrow( + 'Response body too large', + ); + + const dispatcher = (fetchImpl.mock.calls[0]![1] as RequestInit).dispatcher; + expect(asUndiciAgent(dispatcher).closed).toBe(true); + }); }); diff --git a/packages/agent-core/src/tools/providers/local-fetch-url.ts b/packages/agent-core/src/tools/providers/local-fetch-url.ts index ffd24223c4..1b8e50844e 100644 --- a/packages/agent-core/src/tools/providers/local-fetch-url.ts +++ b/packages/agent-core/src/tools/providers/local-fetch-url.ts @@ -25,7 +25,7 @@ import { Readability } from '@mozilla/readability'; import { parseHTML as rawParseHTML } from 'linkedom'; import { Agent, type Dispatcher } from 'undici'; -import { isProxyConfigured } from '../../utils/proxy'; +import { isProxyConfigured, makeNoProxyMatcher, resolveNoProxy } from '../../utils/proxy'; import { HttpFetchError, type UrlFetcher, type UrlFetchResult } from '../builtin'; // Readability's .d.ts references the global `Document` type, but this @@ -103,6 +103,8 @@ function isBlockedAddress(address: string): boolean { interface SafeFetchTarget { /** Lowercased hostname with any IPv6 brackets stripped. */ host: string; + /** Effective origin port — explicit, or the scheme default. */ + port: string; /** Validated DNS answers to pin the connection to — absent when no lookup was needed. */ addresses?: LookupAddress[]; } @@ -130,13 +132,14 @@ async function resolveSafeFetchTarget(url: string, allowPrivate: boolean): Promi // Node versions (and not others). Strip them for uniform comparison. const hostRaw = parsed.hostname.toLowerCase(); const host = hostRaw.startsWith('[') && hostRaw.endsWith(']') ? hostRaw.slice(1, -1) : hostRaw; - if (allowPrivate) return { host }; + const port = parsed.port !== '' ? parsed.port : parsed.protocol === 'https:' ? '443' : '80'; + if (allowPrivate) return { host, port }; // IP literals are checked directly and never resolved. if (isIP(host) !== 0) { if (isBlockedAddress(host)) { throw new Error(`Refusing to fetch private address: "${host}"`); } - return { host }; + return { host, port }; } // Literal "localhost" / loopback aliases. if (host === 'localhost' || host.endsWith('.localhost')) { @@ -159,7 +162,7 @@ async function resolveSafeFetchTarget(url: string, allowPrivate: boolean): Promi throw new Error(`Refusing to fetch host "${host}": resolves to private address "${address}".`); } } - return { host, addresses }; + return { host, port, addresses }; } /** @@ -239,6 +242,11 @@ export class LocalFetchURLProvider implements UrlFetcher { if (contentLengthRaw !== null) { const cl = Number(contentLengthRaw); if (Number.isFinite(cl) && cl > this.maxBytes) { + // Drain before throwing: the caller closes per-hop Agents in a + // finally, and an active oversized stream could stall that close. + await response.body?.cancel().catch(() => { + /* already closed */ + }); throw new Error( `Response body too large: ${String(cl)} bytes exceeds maxBytes (${String(this.maxBytes)}).`, ); @@ -317,10 +325,16 @@ export class LocalFetchURLProvider implements UrlFetcher { // IP literals (and allowPrivate mode) need no pin — there is no second // resolution to race. if (target.addresses === undefined) return undefined; - // With an HTTP/SOCKS proxy configured, origin resolution happens on the - // proxy side; a direct-connect pinned Agent would bypass the proxy - // entirely, so pinning only applies to direct connections. - if (isProxyConfigured(process.env)) return undefined; + // Pin only when this request will actually connect directly. When a + // proxy applies, origin DNS happens on the proxy side (nothing local + // to pin) and a direct-connect pinned Agent would bypass the proxy + // entirely. A NO_PROXY bypass still connects directly — keep pinning. + if ( + isProxyConfigured(process.env) && + !makeNoProxyMatcher(resolveNoProxy(process.env))(target.host, target.port) + ) { + return undefined; + } const dispatcher = new Agent({ connect: { lookup: pinnedLookup(target.host, target.addresses) }, }); diff --git a/packages/agent-core/test/tools/providers/local-fetch-url.test.ts b/packages/agent-core/test/tools/providers/local-fetch-url.test.ts index c1001db9c9..ff23ea1d8d 100644 --- a/packages/agent-core/test/tools/providers/local-fetch-url.test.ts +++ b/packages/agent-core/test/tools/providers/local-fetch-url.test.ts @@ -350,4 +350,46 @@ describe('LocalFetchURLProvider connection pinning', () => { expect((fetchImpl.mock.calls[0]![1] as RequestInit).dispatcher).toBeUndefined(); }); + + it('still pins when the request bypasses the proxy via NO_PROXY wildcard', async () => { + vi.stubEnv('http_proxy', 'http://proxy.example:8080'); + vi.stubEnv('no_proxy', '*'); + const fetchImpl = vi.fn().mockResolvedValue(htmlResponse('ok', 'text/plain')); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await provider.fetch('https://example.com/'); + + expect((fetchImpl.mock.calls[0]![1] as RequestInit).dispatcher).toBeInstanceOf(Agent); + }); + + it('still pins when NO_PROXY exempts the target host specifically', async () => { + vi.stubEnv('http_proxy', 'http://proxy.example:8080'); + vi.stubEnv('no_proxy', 'example.com'); + const fetchImpl = vi.fn().mockResolvedValue(htmlResponse('ok', 'text/plain')); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await provider.fetch('https://example.com/'); + + expect((fetchImpl.mock.calls[0]![1] as RequestInit).dispatcher).toBeInstanceOf(Agent); + }); + + it('rejects oversized responses by content-length and still closes the pinned Agent', async () => { + const fetchImpl = vi.fn().mockResolvedValue( + new Response('short', { + status: 200, + headers: { + 'content-type': 'text/plain', + 'content-length': String(11 * 1024 * 1024), + }, + }), + ); + const provider = new LocalFetchURLProvider({ fetchImpl }); + + await expect(provider.fetch('https://example.com/big')).rejects.toThrow( + 'Response body too large', + ); + + const dispatcher = (fetchImpl.mock.calls[0]![1] as RequestInit).dispatcher; + expect(asUndiciAgent(dispatcher).closed).toBe(true); + }); });