-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Prevent ungettable objects from getting gotten during path lookup #13461
Conversation
return false; | ||
} | ||
|
||
let allowableTypes = ['object', 'function', 'string']; |
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.
move to const outside the function
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.
also can just be an object literal with 1s
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.
like:
const ALLOWABLE_TYPES = {
object: true,
function: true,
string: true
}
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.
Will do. Thanks, guys.
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.
Just pushed the update. In the update I am casting undefined
into false
, so let me know if you believe that to be superfluous and I will remove.
I think @mmun wanted a test for |
There's already one here, but now that I'm looking at it again it's only |
K, |
return false; | ||
} | ||
|
||
return !!ALLOWABLE_TYPES[typeof obj]; |
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 don't think the explicit coercion is needed here. It is probably fine either way though...
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.
Just pushed the update for this.
This looks good, though, this is cleanup, not really a BUGFIX. |
…berjs#13461) Prevent ungettable objects from getting gotten during path lookup
This is a follow-up to #13449.
It was observed by @krisselden that doing something like the following…
… would eventually call
get
internally like…… which is no bueno.
To generalize, we should only call
get
from_getPath
with objects (exceptnull
), functions, and strings. I'm also wondering if we should add a deprecation to warn people when they do something like…… so we can eventually add an assertion. Right now that will just return
undefined
.