logger: address review feedback

Author: mertcanaltin

doc/api/logger.md

@@ -359,6 +359,23 @@ consumer.attach();
 // Consumer now receives all log records at 'info' level and above
 ```
 
+### `consumer.detach()`
+
+<!-- YAML
+added: REPLACEME
+-->
+
+Detaches the consumer from log channels. After calling this method, the consumer
+will no longer receive log records.
+
+```js
+const consumer = new JSONConsumer({ level: 'info' });
+consumer.attach();
+// ... later
+consumer.detach();
+// Consumer no longer receives log records
+```
+
 ### `consumer.<level>.enabled`
 
 <!-- YAML

lib/logger.js

@@ -5,7 +5,6 @@ const {
   ObjectAssign,
   ObjectDefineProperty,
   ObjectKeys,
-  SafeSet,
   SymbolFor,
 } = primordials;
 
@@ -76,12 +75,14 @@ function noop() {}
 class LogConsumer {
   /** @type {number} */
   #levelValue;
+  #handlers;
 
   constructor(options = kEmptyObject) {
     validateObject(options, 'options');
     const { level = 'info' } = options;
     validateOneOf(level, 'options.level', LEVEL_NAMES);
     this.#levelValue = LEVELS[level];
+    this.#handlers = { __proto__: null };
 
     // Setup level-specific enabled properties for typo safety
     // Allows consumer.info.enabled instead of consumer.enabled('info')
@@ -98,7 +99,21 @@ class LogConsumer {
   attach() {
     for (const level of LEVEL_NAMES) {
       if (this[level].enabled) {
-        channels[level].subscribe(this.handle.bind(this));
+        const handler = this.handle.bind(this);
+        this.#handlers[level] = handler;
+        channels[level].subscribe(handler);
+      }
+    }
+  }
+
+  /**
+   * Detach this consumer from log channels
+   */
+  detach() {
+    for (const level of LEVEL_NAMES) {
+      if (this.#handlers[level]) {
+        channels[level].unsubscribe(this.#handlers[level]);
+        this.#handlers[level] = undefined;
       }
     }
   }
@@ -164,15 +179,13 @@ class JSONConsumer extends LogConsumer {
 
   handle(record) {
     // Start building JSON manually
-    let json = `{"level":"${record.level}","time":${record.time},"msg":"${record.msg}"`;
+    // record.level is trusted internal value, record.msg is user input and must be escaped
+    let json = `{"level":"${record.level}","time":${record.time},"msg":${JSONStringify(record.msg)}`;
 
     // Add consumer fields
     const consumerFields = this.#fields;
     for (const key of ObjectKeys(consumerFields)) {
-      const value = consumerFields[key];
-      json += typeof value === 'string' ?
-        `,"${key}":"${value}"` :
-        `,"${key}":${JSONStringify(value)}`;
+      json += `,${JSONStringify(key)}:${JSONStringify(consumerFields[key])}`;
     }
 
     // Add pre-serialized bindings
@@ -181,10 +194,7 @@ class JSONConsumer extends LogConsumer {
     // Add log fields
     const fields = record.fields;
     for (const key in fields) {
-      const value = fields[key];
-      json += typeof value === 'string' ?
-        `,"${key}":"${value}"` :
-        `,"${key}":${JSONStringify(value)}`;
+      json += `,${JSONStringify(key)}:${JSONStringify(fields[key])}`;
     }
 
     json += '}\n';
@@ -285,7 +295,7 @@ class Logger {
     const keys = ObjectKeys(bindings);
     for (const key of keys) {
       const serialized = this.#serializeValue(bindings[key], key);
-      result += `,"${key}":${JSONStringify(serialized)}`;
+      result += `,${JSONStringify(key)}:${JSONStringify(serialized)}`;
     }
     return result;
   }
@@ -480,13 +490,8 @@ class Logger {
 
     if (isNativeError(msgOrObj)) {
       msg = msgOrObj.message;
-      // Use err serializer for Error objects
-      const serializedErr = this.#serializers.err ?
-        this.#serializers.err(msgOrObj) :
-        this.#serializeError(msgOrObj);
-
       logFields = {
-        err: serializedErr,
+        err: this.#serializers.err(msgOrObj),
       };
 
       // Apply serializers to additional fields
@@ -508,17 +513,13 @@ class Logger {
       // Apply serializers to object fields
       logFields = this.#applySerializers(restFields);
 
-      // Special handling for err/error fields
+      // Re-serialize err/error fields with err serializer for proper Error handling
       if (logFields.err && isNativeError(restFields.err)) {
-        logFields.err = this.#serializers.err ?
-          this.#serializers.err(restFields.err) :
-          this.#serializeError(restFields.err);
+        logFields.err = this.#serializers.err(restFields.err);
       }
 
       if (logFields.error && isNativeError(restFields.error)) {
-        logFields.error = this.#serializers.err ?
-          this.#serializers.err(restFields.error) :
-          this.#serializeError(restFields.error);
+        logFields.error = this.#serializers.err(restFields.error);
       }
     }
 
@@ -533,46 +534,6 @@ class Logger {
     channel.publish(record);
   }
 
-  /**
-   * Serialize Error object for logging (with recursive cause handling)
-   * @param {object} err - Error object to serialize
-   * @param {Set} [seen] - Set to track circular references
-   * @returns {object|string} Serialized error object or '[Circular]'
-   * @private
-   */
-  #serializeError(err, seen = new SafeSet()) {
-    if (seen.has(err)) {
-      return '[Circular]';
-    }
-    seen.add(err);
-
-    const serialized = {
-      message: err.message,
-      name: err.name,
-      stack: err.stack,
-    };
-
-    if (err.code !== undefined) {
-      serialized.code = err.code;
-    }
-
-    if (err.cause !== undefined) {
-      serialized.cause = isNativeError(err.cause) ?
-        this.#serializeError(err.cause, seen) :
-        err.cause;
-    }
-
-    // Include additional own error properties (avoid prototype chain)
-    const keys = ObjectKeys(err);
-    for (let i = 0; i < keys.length; i++) {
-      const key = keys[i];
-      if (serialized[key] === undefined) {
-        serialized[key] = err[key];
-      }
-    }
-
-    return serialized;
-  }
 }
 
 module.exports = {

test/parallel/test-logger-serializers.js

@@ -354,4 +354,84 @@ describe('Logger serializers', () => {
       assert.strictEqual(log.data.count, 42);
     });
   });
+
+  describe('JSON escaping', () => {
+    it('should properly escape special characters in msg', () => {
+      const stream = new TestStream();
+      const consumer = new JSONConsumer({ stream, level: 'info' });
+      consumer.attach();
+
+      const logger = new Logger({ level: 'info' });
+      logger.info('My invalid json ", "__proto__": true,');
+      consumer.flushSync();
+
+      assert.strictEqual(stream.logs.length, 1);
+      const log = stream.logs[0];
+      assert.strictEqual(log.msg, 'My invalid json ", "__proto__": true,');
+      assert.strictEqual(log.__proto__, undefined);
+    });
+
+    it('should properly escape special characters in field keys', () => {
+      const stream = new TestStream();
+      const consumer = new JSONConsumer({ stream, level: 'info' });
+      consumer.attach();
+
+      const logger = new Logger({ level: 'info' });
+      logger.info({ msg: 'test', ['"break']: 'value' });
+      consumer.flushSync();
+
+      assert.strictEqual(stream.logs.length, 1);
+      const log = stream.logs[0];
+      assert.strictEqual(log['"break'], 'value');
+    });
+
+    it('should properly escape special characters in field values', () => {
+      const stream = new TestStream();
+      const consumer = new JSONConsumer({ stream, level: 'info' });
+      consumer.attach();
+
+      const logger = new Logger({ level: 'info' });
+      logger.info('test', { data: 'value with "quotes" and \nnewline' });
+      consumer.flushSync();
+
+      assert.strictEqual(stream.logs.length, 1);
+      const log = stream.logs[0];
+      assert.strictEqual(log.data, 'value with "quotes" and \nnewline');
+    });
+
+    it('should properly escape special characters in bindings keys', () => {
+      const stream = new TestStream();
+      const consumer = new JSONConsumer({ stream, level: 'info' });
+      consumer.attach();
+
+      const logger = new Logger({ level: 'info' });
+      const child = logger.child({ ['"break']: 'bla' });
+      child.info('test');
+      consumer.flushSync();
+
+      assert.strictEqual(stream.logs.length, 1);
+      const log = stream.logs[0];
+      assert.strictEqual(log['"break'], 'bla');
+    });
+  });
+});
+
+describe('LogConsumer detach', () => {
+  it('should stop receiving logs after detach', () => {
+    const stream = new TestStream();
+    const consumer = new JSONConsumer({ stream, level: 'info' });
+    consumer.attach();
+
+    const logger = new Logger({ level: 'info' });
+
+    logger.info('before detach');
+    consumer.flushSync();
+    assert.strictEqual(stream.logs.length, 1);
+
+    consumer.detach();
+
+    logger.info('after detach');
+    consumer.flushSync();
+    assert.strictEqual(stream.logs.length, 1); // No new log
+  });
 });