Skip to content

fix: avoid override mistake#1939

Merged
mcollina merged 3 commits intopinojs:masterfrom
erights:markm-avoid-override-mistake
Apr 5, 2024
Merged

fix: avoid override mistake#1939
mcollina merged 3 commits intopinojs:masterfrom
erights:markm-avoid-override-mistake

Conversation

@erights
Copy link
Copy Markdown
Contributor

@erights erights commented Apr 3, 2024 •
edited
Loading

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

    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
Copy link
Copy Markdown
Member

mcollina commented Apr 3, 2024

Can you add a test?

@erights
Copy link
Copy Markdown
Contributor Author

erights commented Apr 4, 2024 •
edited
Loading

Done. The added test/browser-early-console-freeze.test.js succeeds with this PR. Without this PR it 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
Copy link
Copy Markdown
Contributor Author

erights commented Apr 4, 2024

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.

@erights erights mentioned this pull request Apr 4, 2024
Open
@kriskowal
Copy link
Copy Markdown

kriskowal commented Apr 4, 2024

@mcollina Thanks for considering this change.

mcollina
mcollina approved these changes Apr 4, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@kriskowal
Copy link
Copy Markdown

kriskowal commented Apr 4, 2024

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
Copy link
Copy Markdown
Member

jsumners commented Apr 5, 2024

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.

@mcollina mcollina merged commit 9c39e0f into pinojs:master Apr 5, 2024
@erights
Copy link
Copy Markdown
Contributor Author

erights commented Apr 5, 2024

Thanks!

@0xpatrickdev 0xpatrickdev mentioned this pull request Apr 8, 2024
Draft
@erights erights mentioned this pull request Apr 8, 2024
Closed
@snyk-io snyk-io bot mentioned this pull request May 22, 2024
Open
This was referenced May 23, 2024
Open
Open
@s-laugh s-laugh mentioned this pull request May 25, 2024
Closed
@leika leika mentioned this pull request May 31, 2024
Open
@davehorton davehorton mentioned this pull request Jun 6, 2024
Open
@shewood shewood mentioned this pull request Jun 7, 2024
Merged
@thiruaws2lab thiruaws2lab mentioned this pull request Jun 12, 2024
Open
@nnsutebu nnsutebu mentioned this pull request Jun 21, 2024
Open
@AlexOnyi AlexOnyi mentioned this pull request Jun 30, 2024
Open
@moishimwe moishimwe mentioned this pull request Jun 30, 2024
Open
@marcelineepi marcelineepi mentioned this pull request Jun 30, 2024
Open
@keemraheem keemraheem mentioned this pull request Jun 30, 2024
Open
@johanbook johanbook mentioned this pull request Jul 1, 2024
Merged
@fakure2020 fakure2020 mentioned this pull request Jul 3, 2024
Open
@sawood14012 sawood14012 mentioned this pull request Jul 17, 2024
Closed
This was referenced Jul 17, 2024
Open
Open
@harshakrithvikk harshakrithvikk mentioned this pull request Jul 24, 2024
Open
@gptkrsh gptkrsh mentioned this pull request Aug 6, 2024
Open
@gptkrsh gptkrsh mentioned this pull request Aug 18, 2024
Open
This was referenced Sep 6, 2024
Open
Open
@IT21298912 IT21298912 mentioned this pull request Sep 13, 2024
Open
@sandaru-madhubhashana sandaru-madhubhashana mentioned this pull request Sep 16, 2024
Open
@it21109126 it21109126 mentioned this pull request Sep 18, 2024
Open
@Blankll Blankll mentioned this pull request Sep 18, 2024
Closed
@MalithMadusankha MalithMadusankha mentioned this pull request Sep 19, 2024
Open
@IT21071652 IT21071652 mentioned this pull request Sep 26, 2024
Open
@gyenoch gyenoch mentioned this pull request Oct 3, 2024
Open
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 6, 2025

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 6, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mcollina mcollina mcollina approved these changes

@jsumners jsumners jsumners approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@erights @mcollina @kriskowal @jsumners