Bug: BaseLogger breaks if Class of logged error has > 1 required constructor arguments

[nickluger]

Describe the bug
This line assumes that an error constructor has only one required parameter:

tslog/src/BaseLogger.ts

Line 280 in 28ef27a

const newError = new ErrorConstructor(error.message);

If that is not the case, this code breaks the invoked error constructor.

To Reproduce

logger.error(new MyErrorWithMultipleRequiredParameters(a, b))

Expected behavior
I don't know what _cloneError is actually doing, but it should respect error constructors that have multiple required parameters, especially if I don't control these. For example the Prisma ORM throws errors with a second required parameter.

Node.js Version
v16.18.1

[nickluger]

Workaround: Go back to 4.7.4. This was introduced by a5a0ac2

[terehov]

Thank you for reporting.
Would that solve the problem for you?

This method is used to clone the error object in order safely manipulate the cloned one (especially for masking secrets) without changing the original one.

  private _cloneError<T extends Error>(error: T): T {
    const ErrorConstructor = error.constructor as new (...args: any[]) => T;
    const errorProperties = Object.getOwnPropertyNames(error);
    const errorArgs = errorProperties.map((propName) => error[propName]);

    const newError = new ErrorConstructor(...errorArgs);
    Object.assign(newError, error);

    for (const propName of errorProperties) {
      const propDesc = Object.getOwnPropertyDescriptor(newError, propName);
      if (propDesc) {
        propDesc.writable = true;
        Object.defineProperty(newError, propName, propDesc);
      }
    }
    return newError;
  }

[nickluger]

Hey, thanks for the reply.

The new version assumes, that object properties map one-on-one and ordered, to constructor arguments (some kind of dynamic copy constructor) This is a heuristic, that could work in my case or for the libraries I use.

But in general, I think, it's not valid to assume all subclasses of Error in the JS ecosystem follow this pattern. I presume there are many libraries, that don't and would still crash tslog.

For my own code, I could follow this pattern, but I cannot control what others throw. I'm using my logger (as most do) to log all kind of unexpected errors, too.

Anyway, in my opinion, nothing I pass to tslog should crash tslog (same as console.log)

Beyond, I don't think there's a way to clone an error by invoking its constructor, without knowing what the constructor was called with initially (which we don't).

Cloning objects, and especially class instances is a non-trivial issue that is not natively supported by JS, as far as I know:

There's structuredClone, but it's only supported since Node 17, so not backward compatible. Also, it does not clone the prototype chain (instanceof no longer works…)

[terehov]

That's exactly the problem here. On the one hand we want to offer secret masking, which in my opinion is essential for logging, on the other hand we don't want to manipulate the original object and therefore need a copy of it and there is no official way of doing so for the error object.
The commit you mentioned was an attempt to solve this bug, when some properties are read-only: #217

Have you any thoughts on how to solve this?

[nickluger]

After reading a bit up on this, I think there's no way to reliably invoke a constructor without static knowledge about it (for reference: https://stackoverflow.com/a/16025595/2465945).

// This contrived error class would break the dynamic constructor invocation from above
// I'm 100% sure, there's something like this in the wild
class ErrorWithRequiredSecond extends Error {
    private x: { [key: string]: any };

    constructor(message: string, json: string) {
        super(message);
        this.message = message;
        this.x = JSON.parse(json);
    }
}

If we don't have to uphold the instanceof constraint, in the log chain, we could just create our own error.

// We have to use the normal Error constructor or some internal error class
const clonedError = new Error(error.message);
// will print the correct name
clonedError.name = error.name;
// or clonedError.prototype = error.prototype, but not sure what implications of this are

// stack is not enumerable, so we need to move it manually, Object.assign won't do
// Some libraries also remove "stack" in production, so we can't be sure it exists at all
// but if so, we don't want to log the stack of the clone either
clonedError.stack = error.stack || "";

Object.assign(clonedError, error);
const errorProperties = Object.getOwnPropertyNames(error);
for (const propName of errorProperties) {
  // .... as above

In this case, I don't think we can have our cake and eat it, too.

The cloned error would look like the original with respect to logging stuff.

If consumers need instanceof or similar you could use your own error class (instead of plain Error, and retain a reference inside like originalError, that you never log, to keep secrets hidden.

[Mordred]

For me it breaks for typeorm QueryFailedError which expects that driverError has toString() method in constructor: https://github.com/typeorm/typeorm/blob/master/src/error/QueryFailedError.ts

TypeError  Cannot read properties of undefined (reading 'toString')
error stack:
  • QueryFailedError.js	new QueryFailedError
	/node_modules/typeorm/error/QueryFailedError.js:12
  • BaseLogger.js	Logger._cloneError
	/node_modules/tslog/dist/esm/BaseLogger.js:221
  • BaseLogger.js	Logger._recursiveCloneAndMaskValuesOfKeys
	/node_modules/tslog/dist/esm/BaseLogger.js:166
  • BaseLogger.js	
	/node_modules/tslog/dist/esm/BaseLogger.js:140
  • 	
	
  • BaseLogger.js	Logger._mask
	/node_modules/tslog/dist/esm/BaseLogger.js:139
  • BaseLogger.js	Logger.log
	/node_modules/tslog/dist/esm/BaseLogger.js:82
  • index.js	Logger.error
	/node_modules/tslog/dist/esm/index.js:26
  • index.js	Object.didEncounterErrors
	/dist/index.js:249
  • requestPipeline.js	
	/node_modules/@apollo/server/dist/esm/requestPipeline.js:275

[terehov]

Please check V4.9.0.
You can reopen, if it didn't help