fix: TypeError on creating properties on immutable objects #671
Conversation
|
novikovfred
changed the title
novikovfred
changed the title
|
@edmundhung review pls |
novikovfred
changed the title
| function canAddProperty( | ||
| obj: Object | Array<unknown>, | ||
| property: string | number, | ||
| ): boolean { | ||
| const propertyStr = | ||
| typeof property === 'number' ? property.toString() : property; | ||
| if (Object.prototype.hasOwnProperty.call(obj, propertyStr)) { | ||
| const descriptor = Object.getOwnPropertyDescriptor(obj, propertyStr); | ||
| if (descriptor && !descriptor.writable) { | ||
| return false; | ||
| } | ||
| } | ||
| if (!Object.isExtensible(obj)) { | ||
| return false; | ||
| } | ||
| if (Object.isSealed(obj) && !(propertyStr in obj)) { | ||
| return false; | ||
| } | ||
| if (Object.isFrozen(obj) && !(propertyStr in obj)) { | ||
| return false; | ||
| } | ||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
I wanna understand better the impact of this change. Can you explain a bit more what each condition is trying to handle?
There was a problem hiding this comment.
As i mentioned in #670 (we can reproduce with FormData as well)currently we can get TypeError if we will try to assign property on immutable object.
Since we validate and get data from those parameters that can be passed by users, including frauders. We wouldn't want to get errors in runtime error because of a user who enters intentionally incorrect data.
If frauder will try to create any param with immutable type at first(example: param=value) and then will try to add some property to it (example: param.child=value) we will get a TypeError.
Of course, I can handle the error, but I would like not to get errors in the application's runtime.
If we wanna support changes of property values i can rework my PR a bit.
With these change I want to check properly if we can add a property to a given object.
Check if the property already exists and is not writable
if (Object.prototype.hasOwnProperty.call(obj, propStr)) {
const descriptor = Object.getOwnPropertyDescriptor(obj, propStr);
if (descriptor && !descriptor.writable) {
return false;
}
}
Check if the object/array is extensible, isExtensible returns a value that indicates whether new properties can be added to an object.
if (!Object.isExtensible(obj)) {
return false;
}
Check if the object/array is sealed and property doesn't exist, isSealed returns true if existing property attributes cannot be modified in an object and new properties cannot be added to the object.
if (Object.isSealed(obj) && !(propStr in obj)) {
return false;
}
Check if the object/array is frozen and property doesn't exist, isFrozen returns true if existing property attributes and values cannot be modified in an object, and new properties cannot be added to the object.
if (Object.isFrozen(obj) && !(propStr in obj)) {
return false;
}
|
Actually the check makes perfect sense to me. We were bitten by that today as well. Someone updated a fieldset (object) via // state: { fieldset: { property: "aValue" }}
form.update({ name: "fieldset", value "a string" })
// state: { fieldset: "a string" }
// a change to the input below crashes with the same error
<input name="fieldset.property" />We should report a warning as long as we're in development, though. We could end up in the very same situation by simply having a bug in our application. |
fix #670