Fix for non-configurable property access#274
Conversation
|
Lol, node crashed with core dump. Try to fix it later. |
Need to look if this node crash can be abused somehow.
| local.Reflect.defineProperty(target, prop, desc); | ||
| } | ||
| } catch (e) { | ||
| // Should not happen. |
There was a problem hiding this comment.
What do you think about that
(...)
} catch (e) {
throwWhenInTest(e); // should not happen
}
(...)The function throwWhenInTest could throw in your CI/local test environment and the errors would not go unnoticed.
Just a suggestion!
There was a problem hiding this comment.
First I think this can't be reached, since getOwnPropertyDescriptor target is never a proxy. def.configurable is allways a own data property and defineProperty can't fail since target can't be a proxy and the property is either non existant or configurable.
Second I don't know if the exception is from host or sandbox, most likely from the sandbox, so I don't want to expose it. It is possible to stringify it in another try catch and pass the string, but I don't know if that is realy necessary.
|
Thanks for looking into #272. |
| const keys = local.Reflect.ownKeys(object); | ||
| for (let i = 0; i < keys.length; i++) { | ||
| const key = keys[i]; | ||
| let desc = local.Reflect.getOwnPropertyDescriptor(object, key); |
There was a problem hiding this comment.
This can actually throw since object could be a proxy.
|
@jan-osch this is a bug, so I look into it. Question is how long it will take to get merged. |
|
Thanks! |
This should fix #272.
When a non-configurable property is requested, it is now copied into the proxy target and therfore shouldn't violate the proxy target handler rules any more.