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 no-prototype-builtins bug in debug node and utils #3394

Merged
merged 3 commits into from
Feb 3, 2022

Conversation

Alkarex
Copy link
Contributor

@Alkarex Alkarex commented Feb 3, 2022

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Proposed changes

Allows messages containing objects with no prototype built-in to be send to a debug node without crashing.

  • In utils.js, constructs such as msg.msg.constructor.name will crash when msg.msg.constructor is null, for instance as a result of Object.create(null).

  • In 21_debug.js, constructs such as msg.hasOwnProperty("status") might make the debug node crash/produce an error if the payload was created with Object.create(null).

This is the case e.g. for ini (to parse INI files), an official NPM node:
https://github.com/npm/ini/blob/4f289946b3bf95f144e849d771f64e4f2aa2737c/lib/ini.js#L63

My Node-RED node node-red-contrib-parser-ini, which is using that library, was hit by this bug and I had to ship a workaround
https://github.com/alexandrainst/node-red-contrib-parser-ini/blob/fe6b1eb4b18fd54459e2505b1c2f54eb0a9c9fec/parser-ini.js#L14 to make it work with the default output msg.payload of the debug node.

The msg.hasOwnProperty("xxx") construct should not be used since ECMAScript 5.1, and there is no guarantee that obj.constructor exists.

ESLint advises in the same direction https://eslint.org/docs/rules/no-prototype-builtins

This patch was produced using the following regex:
Search: \b([\w.]+).hasOwnProperty\(
Replace: hasOwnProperty.call($1,

This could be applied more globally if desired.

Checklist

  • I have read the contribution guidelines
  • For non-bugfix PRs, I have discussed this change on the forum/slack team.
  • I have run grunt to verify the unit tests pass
  • I have added suitable unit tests to cover the new/changed functionality

`msg.hasOwnProperty("status")` might make the debug node crash/produce an error if the payload was created with `Object.create(null)`.
This is the case e.g. for `ini` (to parse INI files), an official NPM node:
https://github.com/npm/ini/blob/4f289946b3bf95f144e849d771f64e4f2aa2737c/lib/ini.js#L63

My Node-RED node `node-red-contrib-parser-ini`, which is using that library, was hit by this bug and I had to ship a workaround
https://github.com/alexandrainst/node-red-contrib-parser-ini/blob/fe6b1eb4b18fd54459e2505b1c2f54eb0a9c9fec/parser-ini.js#L14

The `msg.hasOwnProperty("xxx")` construct should not be used since ECMAScript 5.1.

ESLint advises in the same direction https://eslint.org/docs/rules/no-prototype-builtins

This patch was produced using the following regex:
Search: `\b([\w.]+).hasOwnProperty\(`
Replace: `Object.prototype.hasOwnProperty.call($1, `

This could be applied more gobally if desired.
@Alkarex
Copy link
Contributor Author

Alkarex commented Feb 3, 2022

Such a fix might incidently be slightly helpful for bugs such as #2780

@Alkarex
Copy link
Contributor Author

Alkarex commented Feb 3, 2022

Another example: Same bug, but with overriding hasOwnProperty:

[
    {
        "id": "15a1b09eda5613f2",
        "type": "inject",
        "z": "d9a661f4.ef966",
        "name": "",
        "props": [
            {
                "p": "payload"
            }
        ],
        "repeat": "",
        "crontab": "",
        "once": false,
        "onceDelay": 0.1,
        "topic": "",
        "payload": "{\"Hello\":\"World\",\"hasOwnProperty\":null}",
        "payloadType": "json",
        "x": 120,
        "y": 1200,
        "wires": [
            [
                "bdca2d4e9b10cdd7"
            ]
        ]
    },
    {
        "id": "bdca2d4e9b10cdd7",
        "type": "debug",
        "z": "d9a661f4.ef966",
        "name": "",
        "active": true,
        "tosidebar": true,
        "console": false,
        "tostatus": true,
        "complete": "payload",
        "targetType": "msg",
        "statusVal": "payload",
        "statusType": "auto",
        "x": 290,
        "y": 1200,
        "wires": []
    }
]

image

@coveralls
Copy link

coveralls commented Feb 3, 2022

Coverage Status

Coverage decreased (-0.03%) to 67.306% when pulling 2e1e61d on Alkarex:fix-debug-node-hasOwnProperty into b7bae18 on node-red:master.

@Alkarex Alkarex marked this pull request as draft February 3, 2022 13:03
@Alkarex Alkarex marked this pull request as ready for review February 3, 2022 15:08
@Alkarex Alkarex changed the title Fix bug in debug node due to msg.hasOwnProperty construct Fix no-prototype-builtins bug in debug node and utils Feb 3, 2022
Copy link
Member

@knolleary knolleary 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. Thanks for the fix - with unit tests! All the better for it.

One small issue with the jsdoc - we don't want internal functions showing up in the generated doc for users.

packages/node_modules/@node-red/util/lib/util.js Outdated Show resolved Hide resolved
@Alkarex
Copy link
Contributor Author

Alkarex commented Feb 3, 2022

I passed the tests locally with Node.js 12. I am not sure why the CI fails. Looks unrelated?

@knolleary
Copy link
Member

Yup - unrelated test failure.

@knolleary knolleary merged commit 5293563 into node-red:master Feb 3, 2022
@Alkarex Alkarex deleted the fix-debug-node-hasOwnProperty branch February 3, 2022 16:48
@Steve-Mcl
Copy link
Contributor

Nick, for future reference, should we mark any internal APIs with @private - does the doc generator understand a jsdoc with @private is not for external use?

REF: https://jsdoc.app/tags-private.html

Private members are not shown in the generated output unless JSDoc is run with the -p/--private

That way re can decorate the functions with info to assist at development time?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants