fix(Variables): Properly resolve vars if prototype in path
#8962
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8962 +/- ##
=======================================
Coverage 87.68% 87.69%
=======================================
Files 268 268
Lines 10045 10050 +5
=======================================
+ Hits 8808 8813 +5
Misses 1237 1237
Continue to review full report at Codecov.
|
lib/classes/Variables.js
Outdated
| // security vulnerabilities. | ||
| const pathArray = Array.isArray(result.path) ? result.path : result.path.split('.'); | ||
| if (pathArray.length > 1) { | ||
| _.get(target, pathArray.slice(0, -1))[pathArray.slice(-1)] = result.populated; |
There was a problem hiding this comment.
I think it won't work if object for which property is destined doesn't already exists, e.g.
obj = {};
_get(obj, ['one'])['two'] = 'value'; // CrashThere was a problem hiding this comment.
That's true - I thought about it and I believe in this particular case it's impossible because during assignment, we only deal with paths that are present and point to a value which has a variable that should be resolved. Am I missing something here and we can have a situation where we're resolving variable that will be assigned to a property that didn't exist before?
There was a problem hiding this comment.
Indeed, so maybe in that case let's simplify it to not rely on _.get (_.get has built in mechanism which ensures we will not crash if path doesn't exist, and here I believe we do not need it). e.g.
const obj = target;
for (const property of path) obj = obj[property];There was a problem hiding this comment.
great idea - I've adjusted my implementation
pgrzesik
force-pushed
the
ensure-proper-variable-resolution-if-prototype-property-is-involved
branch
from
3e58ed8
to
b7fd7c0
Compare
lib/classes/Variables.js
Outdated
| // We could not use direct `_.set` as due to security measures, it did not allow setting values | ||
| // if `prototype` was a part of the path and that caused an infinite loop. In our use-case, there are no | ||
| // security vulnerabilities. |
There was a problem hiding this comment.
I think we can get rid of that comment
At least it's not a real _.set use case (which is more about ensuring that nested objects on they way exist).
Even if _.set would work ok, I think I'd prefer current approach (as _.set confuses what we really deal with here)
lib/classes/Variables.js
Outdated
| for (const property of pathArray.slice(0, -1)) obj = obj[property]; | ||
| obj[pathArray.slice(-1)] = result.populated; |
There was a problem hiding this comment.
In last line we pass array as property name, and it works by a chance of array stringification.
I would probably literally reference last property via e.g. _.last(pathArray)
There was a problem hiding this comment.
good call, changed
pgrzesik
force-pushed
the
ensure-proper-variable-resolution-if-prototype-property-is-involved
branch
from
b7fd7c0
to
ea6544c
Compare
|
sorry 🤦 Pushed now |
pgrzesik
deleted the
ensure-proper-variable-resolution-if-prototype-property-is-involved
branch
Properly resolve variables if
prototypeis present inpathto property where that variable should be replaced. Bug was caused by_.setpreventing updates ifprototypeis present and resulted in infinite loop in variables resolver.Closes: #8956