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
Conversation
@bmeck thank you for the patch 👍 |
There was a problem hiding this 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?
@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 |
e78d3f7
to
6026e15
Compare
index.cjs
Outdated
const desc = Object.getOwnPropertyDescriptor(obj, key); | ||
if (typeof desc !== 'undefined') { | ||
if (desc.get) { | ||
return desc.get; |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
@bmeck friendly ping on this one. |
@bmeck thank you for the contribution. |
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.