fix: improve node-fetch compatibility in error cases MONGOSH-2443

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'
+        );
+      }
+    });
   });
 });

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,

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,44 +237,67 @@ export class TokenSet {
   }
 }
 
+function getCause(err: unknown): Record<string, unknown> | undefined {
+  if (
+    err &&
+    typeof err === 'object' &&
+    'cause' in err &&
+    err.cause &&
+    typeof err.cause === 'object'
+  ) {
+    return err.cause as Record<string, unknown>;
+  }
+}
+
 // [email protected] 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`
 // object, we try to throw a more helpful error.
 export async function improveHTTPResponseBasedError<T>(
   err: T
 ): Promise<T | MongoDBOIDCError> {
-  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 {
+      const statusObject =
+        'status' in cause ? cause : (err as Record<string, unknown>);
+      if (!statusObject.status) return err;
+
       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<string, unknown> = 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 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 +306,65 @@ export async function improveHTTPResponseBasedError<T>(
   }
   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;
+}