fix: support circular reference when normalizing errors (#4069)

.changeset/tough-buckets-listen.md

@@ -0,0 +1,5 @@
+---
+"@logto/app-insights": patch
+---
+
+fix JSON stringify circular structure issue

packages/app-insights/jest.config.js

@@ -0,0 +1,13 @@
+/** @type {import('jest').Config} */
+const config = {
+  transform: {},
+  coveragePathIgnorePatterns: ['/node_modules/', '/src/__mocks__/'],
+  coverageReporters: ['text-summary', 'lcov'],
+  coverageProvider: 'v8',
+  roots: ['./lib'],
+  moduleNameMapper: {
+    '^(chalk|inquirer)$': '<rootDir>/../shared/lib/esm/module-proxy.js',
+  },
+};
+
+export default config;

packages/app-insights/package.json

@@ -24,10 +24,12 @@
   "scripts": {
     "precommit": "lint-staged",
     "build": "rm -rf lib/ && tsc -p tsconfig.build.json",
-    "build:test": "pnpm build",
+    "build:test": "rm -rf lib/ && tsc -p tsconfig.test.json --sourcemap",
     "dev": "tsc -p tsconfig.build.json --watch --preserveWatchOutput --incremental",
     "lint": "eslint --ext .ts src",
     "lint:report": "pnpm lint --format json --output-file report.json",
+    "test:only": "NODE_OPTIONS=--experimental-vm-modules jest",
+    "test": "pnpm build:test && pnpm test:only",
     "prepack": "pnpm build"
   },
   "devDependencies": {
@@ -35,10 +37,12 @@
     "@silverhand/eslint-config-react": "3.0.1",
     "@silverhand/ts-config": "3.0.0",
     "@silverhand/ts-config-react": "3.0.0",
+    "@types/jest": "^29.4.0",
     "@types/node": "^18.11.18",
     "@types/react": "^18.0.31",
     "eslint": "^8.34.0",
     "history": "^5.3.0",
+    "jest": "^29.5.0",
     "lint-staged": "^13.0.0",
     "prettier": "^2.8.2",
     "react": "^18.0.0",

packages/app-insights/src/node.ts

@@ -1,33 +1,7 @@
 import { trySafe } from '@silverhand/essentials';
 import type { TelemetryClient } from 'applicationinsights';
 
-export const normalizeError = (error: unknown) => {
+import { normalizeError } from './normalize-error.js';
-  const errorObject = error instanceof Error ? error : new Error(String(error));
-
-  /**
-   * - Ensure the message if not empty otherwise Application Insights will respond 400
-   *   and the error will not be recorded.
-   * - We stringify error object here since other error properties won't show on the
-   *   ApplicationInsights details page.
-   */
-  const message = JSON.stringify(
-    errorObject,
-    // ApplicationInsights shows call stack, no need to stringify
-    Object.getOwnPropertyNames(errorObject).filter((value) => value !== 'stack')
-  );
-
-  // Ensure we don't mutate the original error
-  const normalized = new Error(message);
-
-  // Manually clone key fields of the error for AppInsights display
-  /* eslint-disable @silverhand/fp/no-mutation */
-  normalized.name = errorObject.name;
-  normalized.stack = errorObject.stack;
-  normalized.cause = errorObject.cause;
-  /* eslint-enable @silverhand/fp/no-mutation */
-
-  return normalized;
-};
 
 class AppInsights {
   client?: TelemetryClient;
@@ -55,7 +29,8 @@ class AppInsights {
     return true;
   }
 
-  trackException(error: unknown) {
+  /** The function is async to avoid blocking the main script and force the use of `await` or `void`. */
+  async trackException(error: unknown) {
     this.client?.trackException({ exception: normalizeError(error) });
   }
 }

packages/app-insights/src/normalize-error.test.ts

@@ -0,0 +1,47 @@
+import { normalizeError } from './normalize-error.js';
+
+describe('normalizeError()', () => {
+  it('should create a new `Error` object with the stringified error as the message', () => {
+    class CustomError extends Error {
+      name = 'CustomError';
+      foo = 'bar';
+    }
+
+    const error = new CustomError('test');
+    const normalized = normalizeError(error);
+    expect(normalized).not.toBe(error);
+    expect(normalized.message).toBe('{"message":"test","name":"CustomError","foo":"bar"}');
+  });
+
+  it('should use the string constructor if the error is not an `Error` instance', () => {
+    const error = 'test';
+    const normalized = normalizeError(error);
+    expect(normalized).not.toBe(error);
+    expect(normalized.message).toBe('"test"');
+  });
+
+  it('should be able to handle circular references and stringify the "stack" key for non-error objects', () => {
+    const circularObject: { message: string; circular?: unknown; stack: string } = {
+      message: 'test',
+      stack: 'foo',
+    };
+    // eslint-disable-next-line @silverhand/fp/no-mutation
+    circularObject.circular = circularObject;
+
+    const normalized = normalizeError(circularObject);
+    expect(normalized).not.toBe(circularObject);
+    expect(normalized.message).toBe('{"message":"test","stack":"foo","circular":"[Circular ~]"}');
+  });
+
+  it('should be able to handle circular error classes', () => {
+    class CircularError extends Error {
+      name = 'CircularError';
+      circular = this;
+    }
+
+    const circularError = new CircularError();
+    const normalized = normalizeError(circularError);
+    expect(normalized).not.toBe(circularError);
+    expect(normalized.message).toBe('{"name":"CircularError","circular":"[Circular ~]"}');
+  });
+});

packages/app-insights/src/normalize-error.ts

@@ -0,0 +1,97 @@
+import { trySafe } from '@silverhand/essentials';
+
+const transformedKey = Symbol('Indicates an object is transformed from an `Error` instance');
+
+// eslint-disable-next-line @typescript-eslint/ban-types
+const isObject = (value: unknown): value is object => typeof value === 'object' && value !== null;
+
+// Edited from https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Cyclic_object_value#circular_references
+function getCircularReplacer() {
+  const ancestors: unknown[] = [];
+  const transformedMap = new WeakMap<Error, unknown>();
+
+  const transformErrorValue = (value: unknown) => {
+    // Special handling for `Error` instances since they have non-enumerable properties
+    if (value instanceof Error) {
+      if (!transformedMap.has(value)) {
+        const transformed: Record<string | symbol, unknown> = {};
+
+        for (const key of Object.getOwnPropertyNames(value)) {
+          // @ts-expect-error getOwnPropertyNames() returns the valid keys
+          // eslint-disable-next-line @silverhand/fp/no-mutation
+          transformed[key] = value[key];
+        }
+
+        // eslint-disable-next-line @silverhand/fp/no-mutation
+        transformed[transformedKey] = true;
+
+        transformedMap.set(value, transformed);
+      }
+
+      return transformedMap.get(value);
+    }
+
+    return value;
+  };
+
+  return function (this: unknown, key: string, value: unknown) {
+    // Ignore `stack` property since ApplicationInsights will show it
+    if (
+      isObject(this) &&
+      Object.prototype.hasOwnProperty.call(this, transformedKey) &&
+      key === 'stack'
+    ) {
+      return;
+    }
+
+    if (!isObject(value)) {
+      return value;
+    }
+
+    // `this` is the object that value is contained in,
+    // i.e., its direct parent.
+    while (ancestors.length > 0 && ancestors.at(-1) !== this) {
+      // eslint-disable-next-line @silverhand/fp/no-mutating-methods
+      ancestors.pop();
+    }
+
+    const transformed = transformErrorValue(value);
+
+    if (ancestors.includes(transformed)) {
+      return '[Circular ~]';
+    }
+
+    // eslint-disable-next-line @silverhand/fp/no-mutating-methods
+    ancestors.push(transformed);
+    return transformed;
+  };
+}
+
+/**
+ * Clone and stringify error object for ApplicationInsights. The stringified result
+ * will be used as the error message. This is necessary because ApplicationInsights
+ * only shows limited error properties.
+ */
+export const normalizeError = (error: unknown) => {
+  /**
+   * - Ensure the message if not empty otherwise Application Insights will respond 400
+   *   and the error will not be recorded.
+   * - We stringify error object here since other error properties won't show on the
+   *   ApplicationInsights details page.
+   */
+  const message = trySafe(() => JSON.stringify(error, getCircularReplacer())) ?? 'Unknown error';
+
+  // Ensure we don't mutate the original error
+  const normalized = new Error(message);
+
+  if (error instanceof Error) {
+    // Manually clone key fields of the error for AppInsights display
+    /* eslint-disable @silverhand/fp/no-mutation */
+    normalized.name = error.name;
+    normalized.stack = error.stack;
+    normalized.cause = error.cause;
+    /* eslint-enable @silverhand/fp/no-mutation */
+  }
+
+  return normalized;
+};

packages/app-insights/tsconfig.json

@@ -2,7 +2,7 @@
   "extends": "@silverhand/ts-config-react/tsconfig.base",
   "compilerOptions": {
     "outDir": "lib",
-    "types": ["node"]
+    "types": ["node", "jest"],
   },
   "include": [
     "src"

packages/app-insights/tsconfig.test.json

@@ -0,0 +1,8 @@
+{
+  "extends": "./tsconfig.build",
+  "compilerOptions": {
+    "isolatedModules": false,
+    "allowJs": true,
+  },
+  "include": ["src"]
+}

packages/cloud/src/middleware/with-error-report.ts

@@ -8,7 +8,7 @@ import type { BaseContext, NextFunction } from '@withtyped/server';
 export default function withErrorReport<InputContext extends BaseContext>() {
   return async (context: InputContext, next: NextFunction<InputContext>) => {
     await tryThat(next(context), (error) => {
-      appInsights.trackException(error);
+      void appInsights.trackException(error);
       throw error;
     });
   };

packages/core/src/app/init.ts

@@ -39,7 +39,7 @@ export default async function initApp(app: Koa): Promise<void> {
 
     const tenant = await trySafe(tenantPool.get(tenantId), (error) => {
       ctx.status = error instanceof TenantNotFoundError ? 404 : 500;
-      appInsights.trackException(error);
+      void appInsights.trackException(error);
     });
 
     if (!tenant) {
@@ -52,7 +52,7 @@ export default async function initApp(app: Koa): Promise<void> {
       tenant.requestEnd();
     } catch (error: unknown) {
       tenant.requestEnd();
-      appInsights.trackException(error);
+      void appInsights.trackException(error);
 
       throw error;
     }

packages/core/src/caches/index.ts

@@ -18,7 +18,7 @@ export class RedisCache implements CacheStore {
         url: conditional(!yes(redisUrl) && redisUrl),
       });
       this.client.on('error', (error) => {
-        appInsights.trackException(error);
+        void appInsights.trackException(error);
       });
     }
   }

packages/core/src/middleware/koa-error-handler.ts

@@ -21,7 +21,7 @@ export default function koaErrorHandler<StateT, ContextT, BodyT>(): Middleware<
       }
 
       // Report all exceptions to ApplicationInsights
-      appInsights.trackException(error);
+      void appInsights.trackException(error);
 
       if (error instanceof RequestError) {
         ctx.status = error.status;

pnpm-lock.yaml

@@ -67,6 +67,9 @@ importers:
       '@silverhand/ts-config-react':
         specifier: 3.0.0
         version: 3.0.0(typescript@5.0.2)
+      '@types/jest':
+        specifier: ^29.4.0
+        version: 29.4.0
       '@types/node':
         specifier: ^18.11.18
         version: 18.11.18
@@ -79,6 +82,9 @@ importers:
       history:
         specifier: ^5.3.0
         version: 5.3.0
+      jest:
+        specifier: ^29.5.0
+        version: 29.5.0(@types/node@18.11.18)
       lint-staged:
         specifier: ^13.0.0
         version: 13.0.0
@@ -9318,7 +9324,7 @@ packages:
   /@types/is-ci@3.0.0:
     resolution: {integrity: sha512-Q0Op0hdWbYd1iahB+IFNQcWXFq4O0Q5MwQP7uN0souuQ4rPg1vEYcnIOfr1gY+M+6rc8FGoRaBO1mOOvL29sEQ==}
     dependencies:
-      ci-info: 3.5.0
+      ci-info: 3.8.0
     dev: true
 
   /@types/istanbul-lib-coverage@2.0.4:
@@ -10632,10 +10638,6 @@ packages:
       mitt: 3.0.0
     dev: true
 
-  /ci-info@3.5.0:
-    resolution: {integrity: sha512-yH4RezKOGlOhxkmhbeNuC4eYZKAUsEaGtBuBzDDP1eFUKiccDWzBABxBfOx31IDwDIXMTxWuwAxUGModvkbuVw==}
-    dev: true
-
   /ci-info@3.8.0:
     resolution: {integrity: sha512-eXTggHWSooYhq49F2opQhuHWgzucfF2YgODK4e1566GQs5BIfP30B0oenwBJHfWxAs2fyPB1s7Mg949zLf61Yw==}
     engines: {node: '>=8'}
@@ -11289,6 +11291,17 @@ packages:
   /dayjs@1.11.6:
     resolution: {integrity: sha512-zZbY5giJAinCG+7AGaw0wIhNZ6J8AhWuSXKvuc1KAyMiRsvGQWqh4L+MomvhdAYjN+lqvVCMq1I41e3YHvXkyQ==}
 
+  /debug@3.2.7:
+    resolution: {integrity: sha512-CFjzYYAi4ThfiQvizrFQevTTXHtnCqWfe7x1AhgEscTz6ZbLbfoLRLPugTQyBth6f8ZERVUSyWHFD/7Wu4t1XQ==}
+    peerDependencies:
+      supports-color: '*'
+    peerDependenciesMeta:
+      supports-color:
+        optional: true
+    dependencies:
+      ms: 2.1.3
+    dev: true
+
   /debug@3.2.7(supports-color@5.5.0):
     resolution: {integrity: sha512-CFjzYYAi4ThfiQvizrFQevTTXHtnCqWfe7x1AhgEscTz6ZbLbfoLRLPugTQyBth6f8ZERVUSyWHFD/7Wu4t1XQ==}
     peerDependencies:
@@ -11830,7 +11843,7 @@ packages:
   /eslint-import-resolver-node@0.3.7:
     resolution: {integrity: sha512-gozW2blMLJCeFpBwugLTGyvVjNoeo1knonXAcatC6bjPBZitotxdWf7Gimr25N4c0AAOo4eOUfaG82IJPDpqCA==}
     dependencies:
-      debug: 3.2.7(supports-color@5.5.0)
+      debug: 3.2.7
       is-core-module: 2.11.0
       resolve: 1.22.1
     transitivePeerDependencies:
@@ -11879,7 +11892,7 @@ packages:
         optional: true
     dependencies:
       '@typescript-eslint/parser': 5.57.0(eslint@8.34.0)(typescript@5.0.2)
-      debug: 3.2.7(supports-color@5.5.0)
+      debug: 3.2.7
       eslint: 8.34.0
       eslint-import-resolver-node: 0.3.7
       eslint-import-resolver-typescript: 3.5.4(eslint-plugin-import@2.27.5)(eslint@8.34.0)
@@ -11931,7 +11944,7 @@ packages:
       array-includes: 3.1.6
       array.prototype.flat: 1.3.1
       array.prototype.flatmap: 1.3.1
-      debug: 3.2.7(supports-color@5.5.0)
+      debug: 3.2.7
       doctrine: 2.1.0
       eslint: 8.34.0
       eslint-import-resolver-node: 0.3.7
@@ -12579,12 +12592,12 @@ packages:
     resolution: {integrity: sha512-dm9s5Pw7Jc0GvMYbshN6zchCA9RgQlzzEZX3vylR9IqFfS8XciblUXOKfW6SiuJ0e13eDYZoZV5wdrev7P3Nwg==}
     engines: {node: ^10.12.0 || >=12.0.0}
     dependencies:
-      flatted: 3.2.5
+      flatted: 3.2.7
       rimraf: 3.0.2
     dev: true
 
-  /flatted@3.2.5:
+  /flatted@3.2.7:
-    resolution: {integrity: sha512-WIWGi2L3DyTUvUrwRKgGi9TwxQMUEqPOPQBVi71R96jZXJdFskXEmf54BoZaS1kknGODoIGASGEzBUYdyMCBJg==}
+    resolution: {integrity: sha512-5nqDSxl8nn5BSNxyR3n4I6eDmbolI6WT+QqR547RwxQapgjQBmtktdP+HTBb/a/zLsbzERTONyUB5pefh5TtjQ==}
     dev: true
 
   /follow-redirects@1.15.1:
@@ -13595,7 +13608,7 @@ packages:
     resolution: {integrity: sha512-ZYvCgrefwqoQ6yTyYUbQu64HsITZ3NfKX1lzaEYdkTDcfKzzCI/wthRRYKkdjHKFVgNiXKAKm65Zo1pk2as/QQ==}
     hasBin: true
     dependencies:
-      ci-info: 3.5.0
+      ci-info: 3.8.0
     dev: true
 
   /is-core-module@2.11.0: