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: avoid legacy accessors #2013

Merged
merged 8 commits into from Oct 7, 2021
Merged

Conversation

bmeck
Copy link
Contributor

@bmeck bmeck commented Aug 22, 2021

These are deprecated and might not exist in every environment. This should also mitigate some false positives as well in object traversals from dynamic property lookups and prototype pollution.

@bcoe
Copy link
Member

bcoe commented Aug 25, 2021

@bmeck thank you for the patch 👍

@bcoe bcoe changed the title Avoid legacy accessors fix: avoid legacy accessors Aug 25, 2021
Copy link
Member

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

Looks to break some tests, any thoughts?

index.cjs Outdated Show resolved Hide resolved
@bmeck
Copy link
Contributor Author

bmeck commented Aug 25, 2021

@bcoe had to go through the spec and do a bit of reading, apparently things aren't as simple as a replace if prototypes are involved

@bmeck bmeck force-pushed the legacy-object-proto-accessors branch from e78d3f7 to 6026e15 Compare August 25, 2021 17:26
index.cjs Outdated
const desc = Object.getOwnPropertyDescriptor(obj, key);
if (typeof desc !== 'undefined') {
if (desc.get) {
return desc.get;
Copy link
Member

Choose a reason for hiding this comment

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

@bmeck can we replace this with:

function lookupGetter(obj, key) {
  const desc = Object.getOwnPropertyDescriptor(obj, key);
  if (typeof desc !== 'undefined') {
    return desc.get;
  }
}

So that coverage remains at 100%? I'm assuming that desc.get will either be the getter function, or undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did a pass to see if it was possible to go through a proto and it only occurs on extremely exotic and tailored Proxy stuff so this replacement should be fine due to it being invoked against the keys/own prop keys.

@bcoe
Copy link
Member

bcoe commented Sep 23, 2021

@bmeck friendly ping on this one.

@bcoe bcoe merged commit adb0d11 into yargs:main Oct 7, 2021
@bcoe
Copy link
Member

bcoe commented Oct 7, 2021

@bmeck thank you for the contribution.

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

2 participants