From ac775dfb8123fd1066ca4f789872e8820eb3ff40 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 7 Aug 2025 18:03:18 +0200 Subject: [PATCH 1/3] fix: improve node-fetch compatibility in error cases MONGOSH-2443 `node-fetch` currently does not return a body that is a web/standard `ReadableStream`, which is what openid-client expects. Since it only makes use of this in error cases, no additional cases are broken, but the error message is highly unhelpful in these cases. Unfortunately, it is not trivial to resolve this without fully wrapping the `node-fetch`, so this provides a limited compatibility layer rather than a full-featured one. --- src/plugin.spec.ts | 49 ++++++++++++++++++- src/plugin.ts | 3 +- src/util.ts | 116 ++++++++++++++++++++++++++++++++++++++------- 3 files changed, 149 insertions(+), 19 deletions(-) diff --git a/src/plugin.spec.ts b/src/plugin.spec.ts index c5a4f7c..af03e77 100644 --- a/src/plugin.spec.ts +++ b/src/plugin.spec.ts @@ -28,7 +28,11 @@ import { verifySuccessfulAuthCodeFlowLog } from '../test/log-hook-verification-h import { automaticRefreshTimeoutMS, MongoDBOIDCPluginImpl } from './plugin'; import sinon from 'sinon'; import { publicPluginToInternalPluginMap_DoNotUseOutsideOfTests } from './api'; -import type { Server as HTTPServer } from 'http'; +import type { + Server as HTTPServer, + ServerResponse, + IncomingMessage, +} from 'http'; import { createServer as createHTTPServer } from 'http'; import type { AddressInfo } from 'net'; import type { @@ -1239,6 +1243,7 @@ describe('OIDC plugin (local OIDC provider)', function () { describe('OIDC plugin (mock OIDC provider)', function () { let provider: OIDCMockProvider; let getTokenPayload: OIDCMockProviderConfig['getTokenPayload']; + let overrideRequestHandler: OIDCMockProviderConfig['overrideRequestHandler']; let additionalIssuerMetadata: OIDCMockProviderConfig['additionalIssuerMetadata']; let receivedHttpRequests: string[] = []; const tokenPayload = { @@ -1264,8 +1269,13 @@ describe('OIDC plugin (mock OIDC provider)', function () { additionalIssuerMetadata() { return additionalIssuerMetadata?.() ?? {}; }, - overrideRequestHandler(url: string) { + overrideRequestHandler( + url: string, + req: IncomingMessage, + res: ServerResponse + ) { receivedHttpRequests.push(url); + return overrideRequestHandler?.(url, req, res); }, }); }); @@ -1277,6 +1287,7 @@ describe('OIDC plugin (mock OIDC provider)', function () { beforeEach(function () { receivedHttpRequests = []; getTokenPayload = () => tokenPayload; + overrideRequestHandler = undefined; additionalIssuerMetadata = undefined; }); @@ -1532,5 +1543,39 @@ describe('OIDC plugin (mock OIDC provider)', function () { statusText: 'Internal Server Error', }); }); + + it('handles JSON failure responses from the IDP', async function () { + overrideRequestHandler = (url, req, res) => { + if (new URL(url).pathname.endsWith('/token')) { + res.writeHead(400, { 'Content-Type': 'application/json' }); + res.end( + JSON.stringify({ + error: 'jsonfailure', + error_description: 'failure text', + }) + ); + } + }; + const plugin = createMongoDBOIDCPlugin({ + openBrowserTimeout: 60_000, + openBrowser: fetchBrowser, + allowedFlows: ['auth-code'], + redirectURI: 'http://localhost:0/callback', + customFetch: sinon.stub().callsFake(fetch), + }); + + try { + await requestToken(plugin, { + issuer: provider.issuer, + clientId: 'mockclientid', + requestScopes: [], + }); + expect.fail('missed exception'); + } catch (err: any) { + expect(err.message).to.equal( + 'server responded with an error in the response body: caused by HTTP response 400 : jsonfailure, failure text' + ); + } + }); }); }); diff --git a/src/plugin.ts b/src/plugin.ts index c6187cb..d19d533 100644 --- a/src/plugin.ts +++ b/src/plugin.ts @@ -12,6 +12,7 @@ import { getRefreshTokenId, improveHTTPResponseBasedError, messageFromError, + nodeFetchCompat, normalizeObject, throwIfAborted, TokenSet, @@ -405,7 +406,7 @@ export class MongoDBOIDCPluginImpl implements MongoDBOIDCPlugin { this.logger.emit('mongodb-oidc-plugin:outbound-http-request', { url }); try { - const response = await this.doFetch(url, init); + const response = nodeFetchCompat(await this.doFetch(url, init)); this.logger.emit('mongodb-oidc-plugin:outbound-http-request-completed', { url, status: response.status, diff --git a/src/util.ts b/src/util.ts index abde002..cb4202a 100644 --- a/src/util.ts +++ b/src/util.ts @@ -6,6 +6,7 @@ import type { } from 'openid-client'; import { MongoDBOIDCError, type OIDCAbortSignal } from './types'; import { createHash, randomBytes } from 'crypto'; +import type { Readable } from 'stream'; class AbortError extends Error { constructor() { @@ -236,6 +237,18 @@ export class TokenSet { } } +function getCause(err: unknown): Record | undefined { + if ( + err && + typeof err === 'object' && + 'cause' in err && + err.cause && + typeof err.cause === 'object' + ) { + return err.cause as Record; + } +} + // openid-client@6.x has reduced error messages for HTTP errors significantly, reducing e.g. // an HTTP error to just a simple 'unexpect HTTP response status code' message, without // further diagnostic information. So if the `cause` of an `err` object is a fetch `Response` @@ -243,37 +256,46 @@ export class TokenSet { export async function improveHTTPResponseBasedError( err: T ): Promise { - if ( - err && - typeof err === 'object' && - 'cause' in err && - err.cause && - typeof err.cause === 'object' && - 'status' in err.cause && - 'statusText' in err.cause && - 'text' in err.cause && - typeof err.cause.text === 'function' - ) { + // Note: `err.cause` can either be an `Error` object itself, or a `Response`, or a JSON HTTP response body + const cause = getCause(err); + if (cause) { try { let body = ''; try { - body = await err.cause.text(); + if ('text' in cause && typeof cause.text === 'function') + body = await cause.text(); // Handle the `Response` case } catch { // ignore } let errorMessageFromBody = ''; try { - const parsed = JSON.parse(body); + let parsed: Record = cause; + try { + parsed = JSON.parse(body); + } catch { + // ignore, and maybe `parsed` already contains the parsed JSON body anyway + } errorMessageFromBody = - ': ' + String(parsed.error_description || parsed.error || ''); + ': ' + + [parsed.error, parsed.error_description] + .filter(Boolean) + .map(String) + .join(', '); } catch { // ignore } if (!errorMessageFromBody) errorMessageFromBody = `: ${body}`; + + const statusObject = + 'status' in cause ? cause : (err as Record); + const statusTextInsert = + 'statusText' in statusObject + ? `(${String(statusObject.statusText)})` + : ''; return new MongoDBOIDCError( `${errorString(err)}: caused by HTTP response ${String( - err.cause.status - )} (${String(err.cause.statusText)})${errorMessageFromBody}`, + statusObject.status + )} ${statusTextInsert}${errorMessageFromBody}`, { codeName: 'HTTPResponseError', cause: err } ); } catch { @@ -282,3 +304,65 @@ export async function improveHTTPResponseBasedError( } return err; } + +// Check whether converting a Node.js `Readable` stream to a web `ReadableStream` +// is possible. We use this for compatibility with fetch() implementations that +// return Node.js `Readable` streams like node-fetch. +export function streamIsNodeReadable(stream: unknown): stream is Readable { + return !!( + stream && + typeof stream === 'object' && + 'pipe' in stream && + typeof stream.pipe === 'function' && + (!('cancel' in stream) || !stream.cancel) + ); +} + +export function nodeFetchCompat( + response: Response & { body: Readable | ReadableStream | null } +): Response { + const notImplemented = (method: string) => + new MongoDBOIDCError(`Not implemented: body.${method}`, { + codeName: 'HTTPBodyShimNotImplemented', + }); + const { body, clone } = response; + if (streamIsNodeReadable(body)) { + // Provide ReadableStream methods that may be used by openid-client + Object.assign( + body, + { + locked: false, + cancel() { + body.resume(); + }, + getReader() { + throw notImplemented('getReader'); + }, + pipeThrough() { + throw notImplemented('pipeThrough'); + }, + pipeTo() { + throw notImplemented('pipeTo'); + }, + tee() { + throw notImplemented('tee'); + }, + values() { + throw notImplemented('values'); + }, + }, + body + ); + Object.assign(response, { + clone: function (this: Response): Response { + // node-fetch replaces `.body` on `.clone()` on *both* + // the original and the cloned Response objects + const cloned = clone.call(this); + nodeFetchCompat(this); + return nodeFetchCompat(cloned); + }, + }); + } + + return response; +} From ea2590e728b15b9694307b5ab75b8357851ac13f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 7 Aug 2025 18:17:21 +0200 Subject: [PATCH 2/3] fixup: do not do HTTP error handling if `status` is not present --- src/util.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/util.ts b/src/util.ts index cb4202a..61f6305 100644 --- a/src/util.ts +++ b/src/util.ts @@ -260,6 +260,10 @@ export async function improveHTTPResponseBasedError( const cause = getCause(err); if (cause) { try { + const statusObject = + 'status' in cause ? cause : (err as Record); + if (!statusObject.status) return err; + let body = ''; try { if ('text' in cause && typeof cause.text === 'function') @@ -286,8 +290,6 @@ export async function improveHTTPResponseBasedError( } if (!errorMessageFromBody) errorMessageFromBody = `: ${body}`; - const statusObject = - 'status' in cause ? cause : (err as Record); const statusTextInsert = 'statusText' in statusObject ? `(${String(statusObject.statusText)})` From 6a841387c358941993295901926fbd2cbb9d6ca2 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 8 Aug 2025 15:56:06 +0200 Subject: [PATCH 3/3] fixup: implement web stream methods --- src/util.ts | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/util.ts b/src/util.ts index 61f6305..5f5986a 100644 --- a/src/util.ts +++ b/src/util.ts @@ -329,27 +329,38 @@ export function nodeFetchCompat( }); const { body, clone } = response; if (streamIsNodeReadable(body)) { + let webStream: ReadableStream | undefined; + const toWeb = () => + webStream ?? (body.constructor as typeof Readable).toWeb?.(body); // Provide ReadableStream methods that may be used by openid-client Object.assign( body, { locked: false, cancel() { + if (webStream) return webStream.cancel(); body.resume(); }, - getReader() { + getReader(...args: Parameters) { + if ((webStream = toWeb())) return webStream.getReader(...args); + throw notImplemented('getReader'); }, - pipeThrough() { + pipeThrough(...args: Parameters) { + if ((webStream = toWeb())) return webStream.pipeThrough(...args); throw notImplemented('pipeThrough'); }, - pipeTo() { + pipeTo(...args: Parameters) { + if ((webStream = toWeb())) return webStream.pipeTo(...args); + throw notImplemented('pipeTo'); }, - tee() { + tee(...args: Parameters) { + if ((webStream = toWeb())) return webStream.tee(...args); throw notImplemented('tee'); }, - values() { + values(...args: Parameters) { + if ((webStream = toWeb())) return webStream.values(...args); throw notImplemented('values'); }, },