Conversation
|
@bmeck thank you for the patch 👍 |
bcoe
left a comment
There was a problem hiding this comment.
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
| const desc = Object.getOwnPropertyDescriptor(obj, key); | ||
| if (typeof desc !== 'undefined') { | ||
| if (desc.get) { | ||
| return desc.get; |
There was a problem hiding this comment.
@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.
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.