Skip to content

Make sure to consume HTTP error response bodies #1173

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Nov 26, 2025
Merged

Make sure to consume HTTP error response bodies #1173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9ab2d4f
Make sure to consume HTTP error response bodies
Nov 26, 2025
05eb2ba
fix: don't cancel response body before reading text in examples
felixweinberger Nov 26, 2025
719604f
revert: remove body.cancel from build script
felixweinberger Nov 26, 2025
17754cb
Merge branch 'main' into egomes/stale-res
felixweinberger Nov 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/client/auth.ts
View file
Open in desktop
Copy link
Contributor

@mattzcarey mattzcarey Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great

Original file line number Diff line number Diff line change
Expand Up @@ -625,10 +625,12 @@ export async function discoverOAuthProtectedResourceMetadata(
});

if (!response || response.status === 404) {
await response?.body?.cancel();
throw new Error(`Resource server does not implement OAuth 2.0 Protected Resource Metadata.`);
}

if (!response.ok) {
await response.body?.cancel();
throw new Error(`HTTP ${response.status} trying to load well-known OAuth protected resource metadata.`);
}
return OAuthProtectedResourceMetadataSchema.parse(await response.json());
Expand Down Expand Up @@ -756,10 +758,12 @@ export async function discoverOAuthMetadata(
});

if (!response || response.status === 404) {
await response?.body?.cancel();
return undefined;
}

if (!response.ok) {
await response.body?.cancel();
throw new Error(`HTTP ${response.status} trying to load well-known OAuth metadata`);
}

Expand Down Expand Up @@ -869,6 +873,7 @@ export async function discoverAuthorizationServerMetadata(
}

if (!response.ok) {
await response.body?.cancel();
// Continue looking for any 4xx response code.
if (response.status >= 400 && response.status < 500) {
continue; // Try next URL
Expand Down
3 changes: 2 additions & 1 deletion src/client/sse.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,8 @@ export class SSEClientTransport implements Transport {

const response = await (this._fetch ?? fetch)(this._endpoint, init);
if (!response.ok) {
const text = await response.text().catch(() => null);

if (response.status === 401 && this._authProvider) {
const { resourceMetadataUrl, scope } = extractWWWAuthenticateParams(response);
this._resourceMetadataUrl = resourceMetadataUrl;
Expand All @@ -272,7 +274,6 @@ export class SSEClientTransport implements Transport {
return this.send(message);
}

const text = await response.text().catch(() => null);
throw new Error(`Error POSTing to endpoint (HTTP ${response.status}): ${text}`);
}
} catch (error) {
Expand Down
27 changes: 18 additions & 9 deletions src/client/streamableHttp.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -582,11 +582,13 @@ describe('StreamableHTTPClientTransport', () => {
ok: false,
status: 401,
statusText: 'Unauthorized',
headers: new Headers()
headers: new Headers(),
text: async () => Promise.reject('dont read my body')
})
.mockResolvedValue({
ok: false,
status: 404
status: 404,
text: async () => Promise.reject('dont read my body')
});

await expect(transport.send(message)).rejects.toThrow(UnauthorizedError);
Expand Down Expand Up @@ -883,7 +885,8 @@ describe('StreamableHTTPClientTransport', () => {
ok: false,
status: 401,
statusText: 'Unauthorized',
headers: new Headers()
headers: new Headers(),
text: async () => Promise.reject('dont read my body')
};
(global.fetch as Mock)
// Initial connection
Expand Down Expand Up @@ -936,7 +939,8 @@ describe('StreamableHTTPClientTransport', () => {
ok: false,
status: 401,
statusText: 'Unauthorized',
headers: new Headers()
headers: new Headers(),
text: async () => Promise.reject('dont read my body')
};
(global.fetch as Mock)
// Initial connection
Expand All @@ -962,7 +966,8 @@ describe('StreamableHTTPClientTransport', () => {
// Fallback should fail to complete the flow
.mockResolvedValue({
ok: false,
status: 404
status: 404,
text: async () => Promise.reject('dont read my body')
});

await expect(transport.send(message)).rejects.toThrow(UnauthorizedError);
Expand All @@ -987,7 +992,8 @@ describe('StreamableHTTPClientTransport', () => {
ok: false,
status: 401,
statusText: 'Unauthorized',
headers: new Headers()
headers: new Headers(),
text: async () => Promise.reject('dont read my body')
};
(global.fetch as Mock)
// Initial connection
Expand All @@ -1013,7 +1019,8 @@ describe('StreamableHTTPClientTransport', () => {
// Fallback should fail to complete the flow
.mockResolvedValue({
ok: false,
status: 404
status: 404,
text: async () => Promise.reject('dont read my body')
});

await expect(transport.send(message)).rejects.toThrow(UnauthorizedError);
Expand All @@ -1026,7 +1033,8 @@ describe('StreamableHTTPClientTransport', () => {
ok: false,
status: 401,
statusText: 'Unauthorized',
headers: new Headers()
headers: new Headers(),
text: async () => Promise.reject('dont read my body')
};

// Create custom fetch
Expand Down Expand Up @@ -1328,7 +1336,8 @@ describe('StreamableHTTPClientTransport', () => {
ok: false,
status: 401,
statusText: 'Unauthorized',
headers: new Headers()
headers: new Headers(),
text: async () => Promise.reject('dont read my body')
};

(global.fetch as Mock)
Expand Down
7 changes: 6 additions & 1 deletion src/client/streamableHttp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,8 @@ export class StreamableHTTPClientTransport implements Transport {
});

if (!response.ok) {
await response.body?.cancel();

if (response.status === 401 && this._authProvider) {
// Need to authenticate
return await this._authThenStart();
Expand Down Expand Up @@ -463,6 +465,8 @@ export class StreamableHTTPClientTransport implements Transport {
}

if (!response.ok) {
const text = await response.text().catch(() => null);

if (response.status === 401 && this._authProvider) {
// Prevent infinite recursion when server returns 401 after successful auth
if (this._hasCompletedAuthFlow) {
Expand Down Expand Up @@ -525,7 +529,6 @@ export class StreamableHTTPClientTransport implements Transport {
}
}

const text = await response.text().catch(() => null);
throw new Error(`Error POSTing to endpoint (HTTP ${response.status}): ${text}`);
}

Expand All @@ -535,6 +538,7 @@ export class StreamableHTTPClientTransport implements Transport {

// If the response is 202 Accepted, there's no body to process
if (response.status === 202) {
await response.body?.cancel();
Copy link
Contributor

@mattzcarey mattzcarey Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome. This line I have been meaning to add for ages

// if the accepted notification is initialized, we start the SSE stream
// if it's supported by the server
if (isInitializedNotification(message)) {
Expand Down Expand Up @@ -609,6 +613,7 @@ export class StreamableHTTPClientTransport implements Transport {
};

const response = await (this._fetch ?? fetch)(this._url, init);
await response.body?.cancel();
Copy link
Contributor

@mattzcarey mattzcarey Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feels weird cancelling the body before checking if response is okay. I'm not super clued up on this I just want to check line 620 will still work as expected.

Copy link
Contributor

@felixweinberger felixweinberger Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question.

I think both .ok and .status should still be available on the response objects after cancellation and unaffected by it.

Tried it out with this script:

(async () => {
        // Fetch something that returns a body
        const response = await fetch('https://httpbin.org/json');

        console.log('Before cancel:');
        console.log('  status:', response.status);
        console.log('  ok:', response.ok);
        console.log('  headers.content-type:', response.headers.get('content-type'));

        // Cancel the body stream
        await response.body?.cancel();
        console.log('\nBody cancelled.\n');

        console.log('After cancel:');
        console.log('  status:', response.status);
        console.log('  ok:', response.ok);
        console.log('  headers.content-type:', response.headers.get('content-type'));

        // Try to read the body now - this should fail
        console.log('\nTrying to read body after cancel:');
        try {
          const text = await response.text();
          console.log('  text:', text.substring(0, 50));
        } catch (e) {
          console.log('  ERROR:', e.message);
        }
      })();

Which gave the following output:

Before cancel:
 status: 200
 ok: true
 headers.content-type: application/json

Body cancelled.

After cancel:
 status: 200
 ok: true
 headers.content-type: application/json

Trying to read body after cancel:
 ERROR: Body is unusable: Body has already been read

So this looks good to me.

We could put 2 cancels, one inside the 405 check and then one at the end of the function, but this seems fine.


// We specifically handle 405 as a valid response according to the spec,
// meaning the server does not support explicit session termination
Expand Down
3 changes: 2 additions & 1 deletion src/examples/server/elicitationUrlExample.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,8 @@ const tokenVerifier = {
});

if (!response.ok) {
throw new Error(`Invalid or expired token: ${await response.text()}`);
const text = await response.text().catch(() => null);
throw new Error(`Invalid or expired token: ${text}`);
}

const data = await response.json();
Expand Down
3 changes: 2 additions & 1 deletion src/examples/server/simpleStreamableHttp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,8 @@ if (useOAuth) {
});

if (!response.ok) {
throw new Error(`Invalid or expired token: ${await response.text()}`);
const text = await response.text().catch(() => null);
throw new Error(`Invalid or expired token: ${text}`);
}

const data = await response.json();
Expand Down
4 changes: 4 additions & 0 deletions src/server/auth/providers/proxyProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ export class ProxyOAuthServerProvider implements OAuthServerProvider {
},
body: params.toString()
});
await response.body?.cancel();

if (!response.ok) {
throw new ServerError(`Token revocation failed: ${response.status}`);
Expand All @@ -107,6 +108,7 @@ export class ProxyOAuthServerProvider implements OAuthServerProvider {
});

if (!response.ok) {
await response.body?.cancel();
throw new ServerError(`Client registration failed: ${response.status}`);
}

Expand Down Expand Up @@ -181,6 +183,7 @@ export class ProxyOAuthServerProvider implements OAuthServerProvider {
});

if (!response.ok) {
await response.body?.cancel();
throw new ServerError(`Token exchange failed: ${response.status}`);
}

Expand Down Expand Up @@ -221,6 +224,7 @@ export class ProxyOAuthServerProvider implements OAuthServerProvider {
});

if (!response.ok) {
await response.body?.cancel();
throw new ServerError(`Token refresh failed: ${response.status}`);
}

Expand Down
Loading