Skip to content

Conversation

@abetomo
Copy link
Collaborator

@abetomo abetomo commented Oct 11, 2019

Pull Request

Problem

#1073

Solution

Revert #1059

ChangeLog

@abetomo abetomo requested a review from shadowspawn October 11, 2019 03:14
index.js Outdated
}

if (this._execs.has(name)) {
if (this._execs[name]) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previously it was the following code.

if (this._execs[name] && typeof this._execs[name] !== 'function') {

The function check was deleted because it was a code when it was an Array.
reference: #249

> a = []; a['filter']
[Function: filter]
> o = {}; o['filter']
undefined

Copy link
Collaborator

Choose a reason for hiding this comment

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

The function check is to avoid treating built-in functions like toString as though they are subcommands, so I recommend a revert to the full previous code.

(The problem the Set was fixing was correctly handling a built-in property that is not a function, which the old code gets wrong.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about === true?

> o = {}; o['toString'] === true
false

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was just thinking something similar! Yes. Simple and elegant.

@abetomo
Copy link
Collaborator Author

abetomo commented Oct 11, 2019

test failed: #1059 (comment)

@abetomo abetomo force-pushed the support_node0.10_in_v2 branch from 931e382 to 38b29eb Compare October 11, 2019 03:37
Copy link
Collaborator

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

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

Thanks!

@abetomo
Copy link
Collaborator Author

abetomo commented Oct 11, 2019

I'll do npm publish.

@abetomo abetomo merged commit a591f87 into tj:release/2.x Oct 11, 2019
@abetomo abetomo deleted the support_node0.10_in_v2 branch October 11, 2019 03:59
@shadowspawn
Copy link
Collaborator

Thanks Abetomo. (I think I used publish tag 2_x with the previous 2.x publish, but then deleted it after publish. No need to delete it though. The publish tag needs to look different than a version number because they share same namespace.)

@abetomo
Copy link
Collaborator Author

abetomo commented Oct 11, 2019

I'm sorry!
I was impatient and unpublished 2.20.2.

If you unpublish, you cannot publish again with that version.
Will you release in 2.20.3?

@shadowspawn
Copy link
Collaborator

shadowspawn commented Oct 11, 2019

Bumping it to 2.20.3 is fine. (No worries.)

@shadowspawn
Copy link
Collaborator

This issue was responsibly reported by the Checkmarx Application Security Research Team. It was fixed in v2.20.3 (#1074) and v3.0.2(#1056).

The Checkmarx vulnerability library lists this as: https://devhub.checkmarx.com/cve-details/Cx435a6fda-ca38/

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.

2 participants