Don't swallow fetch TypeError as CORS in non-browser environments (#1595)
Co-authored-by: Felix Weinberger <3823880+felixweinberger@users.noreply.github.com> Co-authored-by: Felix Weinberger <fweinberger@anthropic.com>
This commit is contained in:
bhosmer-ant
GitHub
parent
f9fda807d3
commit
13a0d345c0
1 changed files with 161 additions and 20 deletions
|
|
@ -33,10 +33,34 @@ vi.mock('pkce-challenge', () => ({
|
|||
const mockFetch = vi.fn();
|
||||
globalThis.fetch = mockFetch;
|
||||
|
||||
/**
|
||||
* fetchWithCorsRetry gates its CORS-swallowing heuristic on the `CORS_IS_POSSIBLE` shim constant.
|
||||
* Tests run under the Node shim (`false`), so a fetch TypeError is treated as a real network error
|
||||
* and thrown instead of swallowed. Tests that specifically exercise the browser CORS retry path
|
||||
* call `withBrowserLikeEnvironment()` to flip the mocked constant to `true`. The `afterEach` hook
|
||||
* resets it so a failed assertion can't leak the override into later tests.
|
||||
*/
|
||||
let mockedCorsIsPossible = false;
|
||||
vi.mock('@modelcontextprotocol/client/_shims', async importOriginal => {
|
||||
const actual = await importOriginal<typeof import('@modelcontextprotocol/client/_shims')>();
|
||||
return {
|
||||
...actual,
|
||||
get CORS_IS_POSSIBLE() {
|
||||
return mockedCorsIsPossible;
|
||||
}
|
||||
};
|
||||
});
|
||||
function withBrowserLikeEnvironment(): void {
|
||||
mockedCorsIsPossible = true;
|
||||
}
|
||||
|
||||
describe('OAuth Authorization', () => {
|
||||
beforeEach(() => {
|
||||
mockFetch.mockReset();
|
||||
});
|
||||
afterEach(() => {
|
||||
mockedCorsIsPossible = false;
|
||||
});
|
||||
|
||||
describe('extractWWWAuthenticateParams', () => {
|
||||
it('returns resource metadata url when present', async () => {
|
||||
|
|
@ -131,7 +155,8 @@ describe('OAuth Authorization', () => {
|
|||
expect(url.toString()).toBe('https://resource.example.com/.well-known/oauth-protected-resource');
|
||||
});
|
||||
|
||||
it('returns metadata when first fetch fails but second without MCP header succeeds', async () => {
|
||||
it('returns metadata when first fetch fails but second without MCP header succeeds (browser CORS retry)', async () => {
|
||||
withBrowserLikeEnvironment();
|
||||
// Set up a counter to control behavior
|
||||
let callCount = 0;
|
||||
|
||||
|
|
@ -159,7 +184,8 @@ describe('OAuth Authorization', () => {
|
|||
expect(mockFetch.mock.calls[0]![1]?.headers).toHaveProperty('MCP-Protocol-Version');
|
||||
});
|
||||
|
||||
it('throws an error when all fetch attempts fail', async () => {
|
||||
it('throws an error when all fetch attempts fail (browser, retry throws non-TypeError)', async () => {
|
||||
withBrowserLikeEnvironment();
|
||||
// Set up a counter to control behavior
|
||||
let callCount = 0;
|
||||
|
||||
|
|
@ -177,6 +203,18 @@ describe('OAuth Authorization', () => {
|
|||
expect(mockFetch).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
it('throws TypeError immediately in non-browser environments without retrying', async () => {
|
||||
// In Node.js/Workers, CORS doesn't exist — a TypeError from fetch is a real
|
||||
// network/config error (DNS failure, connection refused, invalid URL) and
|
||||
// should propagate rather than being silently swallowed.
|
||||
mockFetch.mockImplementation(() => Promise.reject(new TypeError('getaddrinfo ENOTFOUND resource.example.com')));
|
||||
|
||||
await expect(discoverOAuthProtectedResourceMetadata('https://resource.example.com')).rejects.toThrow(TypeError);
|
||||
|
||||
// Only one call — no CORS retry attempted
|
||||
expect(mockFetch).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('throws on 404 errors', async () => {
|
||||
mockFetch.mockResolvedValueOnce({
|
||||
ok: false,
|
||||
|
|
@ -348,7 +386,8 @@ describe('OAuth Authorization', () => {
|
|||
expect(url.toString()).toBe('https://resource.example.com/.well-known/oauth-protected-resource');
|
||||
});
|
||||
|
||||
it('falls back when path-aware discovery encounters CORS error', async () => {
|
||||
it('falls back when path-aware discovery encounters CORS error (browser)', async () => {
|
||||
withBrowserLikeEnvironment();
|
||||
// First call (path-aware) fails with TypeError (CORS)
|
||||
mockFetch.mockImplementationOnce(() => Promise.reject(new TypeError('CORS error')));
|
||||
|
||||
|
|
@ -560,7 +599,8 @@ describe('OAuth Authorization', () => {
|
|||
expect(url.toString()).toBe('https://auth.example.com/.well-known/oauth-authorization-server');
|
||||
});
|
||||
|
||||
it('falls back when path-aware discovery encounters CORS error', async () => {
|
||||
it('falls back when path-aware discovery encounters CORS error (browser)', async () => {
|
||||
withBrowserLikeEnvironment();
|
||||
// First call (path-aware) fails with TypeError (CORS)
|
||||
mockFetch.mockImplementationOnce(() => Promise.reject(new TypeError('CORS error')));
|
||||
|
||||
|
|
@ -591,7 +631,8 @@ describe('OAuth Authorization', () => {
|
|||
});
|
||||
});
|
||||
|
||||
it('returns metadata when first fetch fails but second without MCP header succeeds', async () => {
|
||||
it('returns metadata when first fetch fails but second without MCP header succeeds (browser CORS retry)', async () => {
|
||||
withBrowserLikeEnvironment();
|
||||
// Set up a counter to control behavior
|
||||
let callCount = 0;
|
||||
|
||||
|
|
@ -619,7 +660,8 @@ describe('OAuth Authorization', () => {
|
|||
expect(mockFetch.mock.calls[0]![1]?.headers).toHaveProperty('MCP-Protocol-Version');
|
||||
});
|
||||
|
||||
it('throws an error when all fetch attempts fail', async () => {
|
||||
it('throws an error when all fetch attempts fail (browser, retry throws non-TypeError)', async () => {
|
||||
withBrowserLikeEnvironment();
|
||||
// Set up a counter to control behavior
|
||||
let callCount = 0;
|
||||
|
||||
|
|
@ -637,7 +679,8 @@ describe('OAuth Authorization', () => {
|
|||
expect(mockFetch).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
it('returns undefined when both CORS requests fail in fetchWithCorsRetry', async () => {
|
||||
it('returns undefined when both CORS requests fail in fetchWithCorsRetry (browser)', async () => {
|
||||
withBrowserLikeEnvironment();
|
||||
// fetchWithCorsRetry tries with headers (fails with CORS), then retries without headers (also fails with CORS)
|
||||
// simulating a 404 w/o headers set. We want this to return undefined, not throw TypeError
|
||||
mockFetch.mockImplementation(() => {
|
||||
|
|
@ -827,7 +870,8 @@ describe('OAuth Authorization', () => {
|
|||
await expect(discoverAuthorizationServerMetadata('https://mcp.example.com')).rejects.toThrow('HTTP 500');
|
||||
});
|
||||
|
||||
it('handles CORS errors with retry', async () => {
|
||||
it('handles CORS errors with retry (browser)', async () => {
|
||||
withBrowserLikeEnvironment();
|
||||
// First call fails with CORS
|
||||
mockFetch.mockImplementationOnce(() => Promise.reject(new TypeError('CORS error')));
|
||||
|
||||
|
|
@ -883,7 +927,8 @@ describe('OAuth Authorization', () => {
|
|||
});
|
||||
});
|
||||
|
||||
it('returns undefined when all URLs fail with CORS errors', async () => {
|
||||
it('returns undefined when all URLs fail with CORS errors (browser)', async () => {
|
||||
withBrowserLikeEnvironment();
|
||||
// All fetch attempts fail with CORS errors (TypeError)
|
||||
mockFetch.mockImplementation(() => Promise.reject(new TypeError('CORS error')));
|
||||
|
||||
|
|
@ -894,6 +939,18 @@ describe('OAuth Authorization', () => {
|
|||
// Verify that all discovery URLs were attempted
|
||||
expect(mockFetch).toHaveBeenCalledTimes(6); // 3 URLs × 2 attempts each (with and without headers)
|
||||
});
|
||||
|
||||
it('throws TypeError in non-browser environments instead of silently falling through (network failure)', async () => {
|
||||
// In Node.js, a TypeError from fetch is a real error (DNS/connection), not CORS.
|
||||
// Swallowing it and returning undefined would cause the caller to silently fall
|
||||
// through to the next discovery URL, masking the actual network failure.
|
||||
mockFetch.mockImplementation(() => Promise.reject(new TypeError('getaddrinfo ENOTFOUND auth.example.com')));
|
||||
|
||||
await expect(discoverAuthorizationServerMetadata('https://auth.example.com/tenant1')).rejects.toThrow(TypeError);
|
||||
|
||||
// Only one call — no CORS retry attempted in non-browser environments
|
||||
expect(mockFetch).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
});
|
||||
|
||||
describe('discoverOAuthServerInfo', () => {
|
||||
|
|
@ -1006,6 +1063,15 @@ describe('OAuth Authorization', () => {
|
|||
// Verify the override URL was used instead of the default well-known path
|
||||
expect(mockFetch.mock.calls[0]![0].toString()).toBe(overrideUrl.toString());
|
||||
});
|
||||
|
||||
it('propagates network failures instead of silently falling back (non-browser)', async () => {
|
||||
// PRM discovery hits a DNS/connection failure. That's a transient reachability problem,
|
||||
// not "server doesn't support RFC 9728" — the caller should see the real error rather
|
||||
// than silently falling back to treating the MCP server URL as the auth server.
|
||||
mockFetch.mockImplementation(() => Promise.reject(new TypeError('getaddrinfo ENOTFOUND resource.example.com')));
|
||||
|
||||
await expect(discoverOAuthServerInfo('https://resource.example.com')).rejects.toThrow(TypeError);
|
||||
});
|
||||
});
|
||||
|
||||
describe('auth with provider authorization server URL caching', () => {
|
||||
|
|
|
|||
Loading…
Reference in a new issue