fix: improve node-fetch compatibility in error cases MONGOSH-2443 (#224)

`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.

Author: addaleax

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 {
@@ -1245,6 +1249,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 = {
@@ -1270,8 +1275,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);
       },
     });
   });
@@ -1283,6 +1293,7 @@ describe('OIDC plugin (mock OIDC provider)', function () {
   beforeEach(function () {
     receivedHttpRequests = [];
     getTokenPayload = () => tokenPayload;
+    overrideRequestHandler = undefined;
     additionalIssuerMetadata = undefined;
   });
 
@@ -1583,5 +1594,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,76 @@ 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)) {
+    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(...args: Parameters<ReadableStream['getReader']>) {
+          if ((webStream = toWeb())) return webStream.getReader(...args);
+
+          throw notImplemented('getReader');
+        },
+        pipeThrough(...args: Parameters<ReadableStream['pipeThrough']>) {
+          if ((webStream = toWeb())) return webStream.pipeThrough(...args);
+          throw notImplemented('pipeThrough');
+        },
+        pipeTo(...args: Parameters<ReadableStream['pipeTo']>) {
+          if ((webStream = toWeb())) return webStream.pipeTo(...args);
+
+          throw notImplemented('pipeTo');
+        },
+        tee(...args: Parameters<ReadableStream['tee']>) {
+          if ((webStream = toWeb())) return webStream.tee(...args);
+          throw notImplemented('tee');
+        },
+        values(...args: Parameters<ReadableStream['values']>) {
+          if ((webStream = toWeb())) return webStream.values(...args);
+          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;
+}