Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix hasOwnProperty usage #1359

Merged
merged 2 commits into from
Mar 23, 2022
Merged

Fix hasOwnProperty usage #1359

merged 2 commits into from
Mar 23, 2022

Conversation

joshkel
Copy link
Contributor

@joshkel joshkel commented Mar 4, 2022

Under certain circumstances, the user's mergingObject may be passed through as is to write. If the mergingObject was created with no prototype (Object.create(null)), then calling hasOwnProperty will fail.

Sample problematic code:

const PinoMS = require('pino-multi-stream');
const pino = require('pino');
const logger = pino({}, pino.multistream([
  { stream: PinoMS.prettyStream({ prettyPrint: { colorize: true, translateTime: 'SYS:standard' } }) }
]));

const example = Object.create(null);
Object.assign(example, { hello: 'world' });
logger.info(example, 'hello world');

(This worked prior to Pino 7.8.0.)

For consistency, I also updated another usage of hasOwnProperty to match the new style.

Under certain circumstances, the user's mergingObject may be passed
through as is to `write`. If the mergingObject was created with no
prototype (`Object.create(null)`), then calling hasOwnProperty will
fail.

Sample problematic code:

```js
const PinoMS = require('pino-multi-stream');
const pino = require('pino');
const logger = pino({}, pino.multistream([
  { stream: PinoMS.prettyStream({ prettyPrint: { colorize: true, translateTime: 'SYS:standard' } }) }
]));

const example = Object.create(null);
Object.assign(example, { hello: 'world' });
logger.info(example, 'hello world');
```

(This worked prior to Pino 7.8.0.)

For consistency,   updated another usage of hasOwnProperty to match the
new style.
Copy link
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.

Good spot! Can you please add a unit test?

@joshkel
Copy link
Contributor Author

joshkel commented Mar 23, 2022

@mcollina Done. I apologize for the delay.

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

Looks good to me.

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

@mcollina mcollina merged commit 572ac0a into pinojs:master Mar 23, 2022
@joshkel joshkel deleted the object-prototype branch March 23, 2022 22:16
@github-actions
Copy link

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 Mar 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants