Repository: pinojs/pino
Pull request #1939 (2024-04-03 19:46:10)

fix: avoid override mistake
The assignment at this location was to a `self` object that inherits from the `console` (or what was the `console` before this code ran). This assignment was use to override properties typically inherited from `console` such as `error`,`warn`,`info`,`debug`,`trace`. However, if the `console` object it inherits from is frozen, as it is in the ses-shim of HardenedJS, then this assignment fails due to the so-called "override mistake".

Changing this assignment to use `Object.defineProperty` instead avoids hitting the override mistake. The additional attributes I include in this change
```js
    writable: true,
    enumerable: true,
    configurable: true,
```
are only there to preserve equivalence with assignment. But feel free to change them to whatever you'd like. Such changes would not affect the purpose of this PR.

mcollina: Can you add a test?
erights: Done. The added `test/browser-early-console-freeze.test.js` succeeds with this PR. Without this PR is fails as follows:

```
test/browser-early-console-freeze.test.js 2> /Users/markmiller/src/ongithub/erights/pino/browser.js:9
test/browser-early-console-freeze.test.js 2> 
self[level]=levelToValue(self.level,rootLogger)>levelToValue(level,rootLogger)?(cov_qeewo7tvl().b[37][0]++,noop):(cov_qeewo7tvl().b[37][1]++,rootLogger[baseLogFunctionSymbol][level]);cov_qeewo7tvl().s[117]++;if(
(cov_qeewo7tvl().b[39][0]++,!opts.transmit)&&(cov_qeewo7tvl().b[39][1]++,self[level]===noop)){cov_qeewo7tvl().b[38][0]++;cov_qeewo7tvl().s[118]++;return;}else{cov_qeewo7tvl().b[38][1]++;}// make sure the log
format is correct
test/browser-early-console-freeze.test.js 2>            ^
test/browser-early-console-freeze.test.js 2> 
test/browser-early-console-freeze.test.js 2> TypeError: Cannot assign to read only property 'error' of object '#<Object>'
test/browser-early-console-freeze.test.js 2>     at set (/Users/markmiller/src/ongithub/erights/pino/browser.js:9:12)
test/browser-early-console-freeze.test.js 2>     at console.setLevel (/Users/markmiller/src/ongithub/erights/pino/browser.js:3:1735)
test/browser-early-console-freeze.test.js 2>     at pino (/Users/markmiller/src/ongithub/erights/pino/browser.js:3:483)
test/browser-early-console-freeze.test.js 2>     at Test.<anonymous> (/Users/markmiller/src/ongithub/erights/pino/test/browser-early-console-freeze.test.js:7:3)
test/browser-early-console-freeze.test.js 2>     at Test.run (/Users/markmiller/src/ongithub/erights/pino/node_modules/tape/lib/test.js:127:28)
test/browser-early-console-freeze.test.js 2>     at Immediate.next (/Users/markmiller/src/ongithub/erights/pino/node_modules/tape/lib/results.js:156:7)
test/browser-early-console-freeze.test.js 2>     at process.processImmediate (node:internal/timers:476:21)
test/browser-early-console-freeze.test.js 2> 
test/browser-early-console-freeze.test.js 2> Node.js v18.19.0
​ FAIL ​ TAP
 ✖ test/browser-early-console-freeze.test.js
```
erights: The
```
TypeError: Cannot assign to read only property 'error' of object
```
is exactly the symptom we saw in our own system that led us here.
kriskowal: @mcollina Thanks for considering this change.
kriskowal: There’s a test failure affecting only Windows. Can I trouble a maintainer to re-run that to verify whether this is a flake?
jsumners: It's the transport tests. I wish I knew why they consistently fail as often as they do. But we can just ignore it for this.

Patch:
diff --git a/browser.js b/browser.js
index f7dd082..04b00ce 100644
--- a/browser.js
+++ b/browser.js
@@ -263,9 +263,14 @@ function getBindingChain (logger) {
 
 function set (self, opts, rootLogger, level) {
   // override the current log functions with either `noop` or the base log function
-  self[level] = levelToValue(self.level, rootLogger) > levelToValue(level, rootLogger)
-    ? noop
-    : rootLogger[baseLogFunctionSymbol][level]
+  Object.defineProperty(self, level, {
+    value: (levelToValue(self.level, rootLogger) > levelToValue(level, rootLogger)
+      ? noop
+      : rootLogger[baseLogFunctionSymbol][level]),
+    writable: true,
+    enumerable: true,
+    configurable: true
+  })
 
   if (!opts.transmit && self[level] === noop) {
     return


Test patch:
diff --git a/test/browser-early-console-freeze.test.js b/test/browser-early-console-freeze.test.js
new file mode 100644
index 0000000..942abfa
--- /dev/null
+++ b/test/browser-early-console-freeze.test.js
@@ -0,0 +1,12 @@
+'use strict'
+Object.freeze(console)
+const test = require('tape')
+const pino = require('../browser')
+
+test('silent level', ({ end, fail, pass }) => {
+  pino({
+    level: 'silent',
+    browser: { }
+  })
+  end()
+})