Close the connection when an oversized fetch is refused (#553)

* Close the connection when an oversized fetch is refused

Fixes #545

A response body refused for exceeding a byte cap was left unread, and
node-fetch holds the connection until the body stream is either consumed or
destroyed. The declared-content-length check refuses before anything
subscribes to the stream, so every over-cap source URL stranded one socket
for as long as the server ran. The streamed-total check already disposed of
the body, because leaving the read loop destroys the stream on the way out;
only the declared-length path leaked. Measured against a loopback server:
the refused connection stays open indefinitely, and destroying the body
closes it at once.

`readCapped` now destroys the body on any refusal, so the invariant holds at
both cap checks and for both callers: the `*-from-url` upload tools and the
capped SPARQL read behind `wikibase-query`. Aborting the `AbortController`
that `fetchFileBytes` owns would close its own connection too, but it is
neither necessary nor available to `postForm`, whose signal belongs to the
caller.

The regression test drives the real client against a loopback server, since
the rest of the suite mocks node-fetch and cannot see a socket, and asserts
that the server's end of the connection goes away.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Close two false-pass routes in the connection-disposal tests

The test server answered any unrecognised path with the chunked over-cap
response, so a mistyped route in the streamed test still got a refusal to
assert on and passed while exercising nothing it named. Unknown paths now
answer 404, which the error-type assertion rejects.

The socket the assertion reads is whichever one the server last served. It
is now cleared before each test, and an assertion with no socket to observe
fails rather than reporting an earlier test's closed connection as its own.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Tighten the changelog entry for the refused-fetch fix

Lead with the three tools a reader scans for, and drop the mechanism the
entry opened with. The old wording also hung the wikibase-query case off
MCP_UPLOAD_MAX_BYTES, which governs only the upload tools, and counted the
leak per URL rather than per call.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Narrow the response body by instanceof rather than asserting it

node-fetch declares the body as the wider NodeJS.ReadableStream, which
has no destroy(). Asserting past that means a body that is not a Node
stream throws a TypeError from inside the catch, replacing the
size-refusal it was reporting — and a TypeError does not rescue to
wiki-side copy-upload, so a routine refusal would surface as an error.
Narrowing by instanceof leaves such a body alone instead.

Also narrows the changelog entry to the two released tools and to the
setting that governs them. The capped SPARQL read passes a timeout
signal, so its connection was freed after a minute rather than held for
the life of the process, and both it and the tool that makes it arrive
unreleased in this same cycle.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: trial <a@b.c>

Author: 3 people

CHANGELOG.md

@@ -22,6 +22,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/)
 ### Fixed
 
 - `update-page` no longer advertises itself as idempotent: in `mode='append'` and `mode='prepend'` it never was, so a client replaying a call whose result never arrived adds the content a second time. A replace resends the same content rather than adding to it.
+- `upload-file-from-url` and `update-file-from-url` no longer leak a connection when they refuse a source URL whose declared size is over `MCP_UPLOAD_MAX_BYTES`. Each refused call held one connection open for as long as the server ran.
 
 ## [0.16.0] - 2026-07-30
 

src/transport/httpFetch.ts

@@ -1,3 +1,4 @@
+import { Readable } from 'node:stream';
 import fetch, { Response, FetchError } from 'node-fetch';
 import { USER_AGENT } from '../runtime/constants.ts';
 import { isErrnoException } from '../errors/isErrnoException.ts';
@@ -193,30 +194,55 @@ export async function postForm(
  * Reads a response body into memory under a byte cap, checked twice: the
  * declared content-length rejects an over-cap body before a byte is read, and
  * the running total catches one that under-declares or declares nothing.
+ *
+ * Either refusal disposes of the body first: the connection is held for as long
+ * as the body stream is neither consumed nor destroyed, so a body left unread
+ * strands a socket until something aborts the request — which, on the upload
+ * path, nothing does.
  */
 async function readCapped(
 	response: Response,
 	maxBytes: number,
 	limitName?: string,
 ): Promise<Buffer> {
-	const declared = Number(response.headers.get('content-length'));
-	if (Number.isFinite(declared) && declared > maxBytes) {
-		throw new FileTooLargeError(declared, maxBytes, limitName);
-	}
-	const chunks: Buffer[] = [];
-	let total = 0;
-	if (response.body !== null) {
-		// node-fetch v3 exposes the body as a Node Readable (async-iterable).
-		// oxlint-disable-next-line typescript/no-unsafe-type-assertion -- node-fetch v3 body is always a Node.js Readable; narrowing to AsyncIterable<Buffer> is safe at this boundary
-		for await (const chunk of response.body as AsyncIterable<Buffer>) {
-			total += chunk.length;
-			if (total > maxBytes) {
-				throw new FileTooLargeError(total, maxBytes, limitName);
+	try {
+		const declared = Number(response.headers.get('content-length'));
+		if (Number.isFinite(declared) && declared > maxBytes) {
+			throw new FileTooLargeError(declared, maxBytes, limitName);
+		}
+		const chunks: Buffer[] = [];
+		let total = 0;
+		if (response.body !== null) {
+			// node-fetch v3 exposes the body as a Node Readable (async-iterable).
+			// oxlint-disable-next-line typescript/no-unsafe-type-assertion -- node-fetch v3 body is always a Node.js Readable; narrowing to AsyncIterable<Buffer> is safe at this boundary
+			for await (const chunk of response.body as AsyncIterable<Buffer>) {
+				total += chunk.length;
+				if (total > maxBytes) {
+					throw new FileTooLargeError(total, maxBytes, limitName);
+				}
+				chunks.push(chunk);
 			}
-			chunks.push(chunk);
 		}
+		return Buffer.concat(chunks);
+	} catch (error) {
+		destroyBody(response);
+		throw error;
+	}
+}
+
+/**
+ * Releases the connection behind a response body that will not be read. Leaving
+ * the read loop above destroys the stream on its way out, but a refusal that
+ * never subscribes to it has to do this itself.
+ */
+function destroyBody(response: Response): void {
+	// node-fetch v3 hands back a Node Readable, but declares it as the wider
+	// NodeJS.ReadableStream, which has no destroy(). Narrowing by instanceof
+	// rather than asserting means a body that is not a Node stream is left
+	// alone instead of throwing over the refusal being reported.
+	if (response.body instanceof Readable) {
+		response.body.destroy();
 	}
-	return Buffer.concat(chunks);
 }
 
 export async function fetchPageHtml(url: string): Promise<string | null> {

tests/transport/httpFetch.disposal.test.ts

@@ -0,0 +1,139 @@
+import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest';
+
+/**
+ * What an over-cap refusal does to the connection is only visible against a
+ * real server: the rest of the httpFetch suite mocks node-fetch wholesale, so
+ * it never sees a socket. node-fetch holds the connection until the response
+ * body is consumed or destroyed, so a refusal that leaves the body untouched
+ * strands the socket. Only the SSRF guard is stubbed, since a loopback
+ * destination is what a local test server is.
+ */
+vi.mock('../../src/transport/ssrfGuard.ts', async () => {
+	const actual = await vi.importActual<typeof import('../../src/transport/ssrfGuard.ts')>(
+		'../../src/transport/ssrfGuard.ts',
+	);
+	return {
+		...actual,
+		assertPublicDestination: vi.fn(async () => [{ address: '127.0.0.1', family: 4 }]),
+		buildPinnedAgent: vi.fn(() => undefined),
+	};
+});
+
+import { createServer, type Server } from 'node:http';
+import type { Socket } from 'node:net';
+import { fetchFileBytes, postForm, FileTooLargeError } from '../../src/transport/httpFetch.ts';
+
+let server: Server;
+let origin: string;
+let openSockets: Set<Socket>;
+let servingSocket: Socket | undefined;
+
+beforeAll(async () => {
+	openSockets = new Set();
+	server = createServer((req, res) => {
+		servingSocket = req.socket;
+		if (req.url === '/small') {
+			res.writeHead(200, { 'Content-Length': '7' });
+			res.end('results');
+			return;
+		}
+		// The over-cap routes send their headers and then stall, never ending the
+		// response. A client that drops such a body without destroying it holds
+		// the socket open for as long as it lives, which is what these tests
+		// measure; one that completes normally would return the socket to the
+		// keep-alive pool and hide the difference.
+		if (req.url === '/declares-too-much') {
+			res.writeHead(200, { 'Content-Length': String(50 * 1024 * 1024) });
+			res.write('x');
+			return;
+		}
+		if (req.url === '/streams-too-much') {
+			res.writeHead(200, { 'Transfer-Encoding': 'chunked' });
+			res.write('x'.repeat(64));
+			return;
+		}
+		// Anything else is a test asking for a route that does not exist. Refusing
+		// it keeps a mistyped path from quietly getting a different route's answer.
+		res.writeHead(404);
+		res.end();
+	});
+	// A client destroying its socket reaches the server as a reset.
+	server.on('connection', (socket) => {
+		openSockets.add(socket);
+		socket.on('error', () => {});
+		socket.on('close', () => openSockets.delete(socket));
+	});
+	await new Promise<void>((resolve) => server.listen(0, '127.0.0.1', resolve));
+	const address = server.address();
+	origin = `http://127.0.0.1:${typeof address === 'object' && address !== null ? address.port : 0}`;
+});
+
+beforeEach(() => {
+	servingSocket = undefined;
+});
+
+afterAll(async () => {
+	for (const socket of openSockets) {
+		socket.destroy();
+	}
+	await new Promise<void>((resolve) => server.close(() => resolve()));
+});
+
+/**
+ * Whether the server's end of the connection goes away once the client is done
+ * with it. No recorded socket means the request never reached the server, so
+ * there is nothing to observe: say so, rather than read a socket an earlier test
+ * left behind and report its closure as this one's.
+ */
+async function connectionClosed(socket: Socket | undefined, withinMs = 1000): Promise<boolean> {
+	if (socket === undefined) {
+		throw new Error('The server served no request, so no connection was observed.');
+	}
+	if (socket.destroyed) {
+		return true;
+	}
+	return await new Promise<boolean>((resolve) => {
+		const timer = setTimeout(() => resolve(false), withinMs);
+		socket.once('close', () => {
+			clearTimeout(timer);
+			resolve(true);
+		});
+	});
+}
+
+describe('an over-cap body refused against a real server', () => {
+	it('closes the connection when the declared content-length is over the cap', async () => {
+		const failure = await fetchFileBytes(`${origin}/declares-too-much`, {
+			maxBytes: 1024,
+		}).catch((error: unknown) => error);
+
+		expect(failure).toBeInstanceOf(FileTooLargeError);
+		expect(await connectionClosed(servingSocket)).toBe(true);
+	});
+
+	it('closes the connection when the streamed body passes the cap', async () => {
+		const failure = await fetchFileBytes(`${origin}/streams-too-much`, { maxBytes: 10 }).catch(
+			(error: unknown) => error,
+		);
+
+		expect(failure).toBeInstanceOf(FileTooLargeError);
+		expect(await connectionClosed(servingSocket)).toBe(true);
+	});
+
+	it('closes the connection when a capped postForm refuses the body', async () => {
+		const failure = await postForm(
+			`${origin}/declares-too-much`,
+			{ query: 'SELECT ?x WHERE {}' },
+			{ maxBytes: 1024 },
+		).catch((error: unknown) => error);
+
+		expect(failure).toBeInstanceOf(FileTooLargeError);
+		expect(await connectionClosed(servingSocket)).toBe(true);
+	});
+
+	it('still returns a body that fits the cap', async () => {
+		const body = await postForm(`${origin}/small`, { query: 'x' }, { maxBytes: 1024 });
+
+		expect(body).toBe('results');
+	});
+});