Support re exp for scuttling avoid props#405
Conversation
|
Why are we supporting this? While trying to integrate the new scuttling feature into MetaMask web app we learned that in its test env where a chrome driver is used, the internals of the driver sets in different occasions random props to the global object as part of its legitimate flow. The scuttling feature takes these props and scuttled them which interferes with the normal flow of the driver. We would like to except those properties but they are random with a recurring pattern Being able to scuttle props by a regex and not a hardcoded string gives us the ability to treat that |
There was a problem hiding this comment.
please document that strings starting with a forward slash are interpreted as regexp
for implementation comparison: https://github.com/IonicaBizau/regex-parser.js/blob/master/lib/index.js#L11-L28
Co-authored-by: kumavis <kumavis@users.noreply.github.com>
| props.push(...Object.getOwnPropertyNames(proto))) | ||
|
|
||
| for (let i = 0; i < extraPropsToAvoid.length; i++) { | ||
| const prop = extraPropsToAvoid[i] |
There was a problem hiding this comment.
Just to make sure: it seems we can trust the input here as coming from the trusted initialization code, right?
| if (avoid instanceof RegExp && avoid.test(prop)) { | ||
| return true | ||
| } | ||
| if (propsToAvoid.has(prop)) { |
There was a problem hiding this comment.
(could) create a test covering that it's not possible to accidentally match the string representation of the regex by setting window['/as.good.a.field.name.as.any/']
There was a problem hiding this comment.
Not too worried about such a scenario
| if (scuttleGlobalThisExceptions) { | ||
| // toString regexps if there's any | ||
| for (let i = 0; i < scuttleGlobalThisExceptions.length; i++) { | ||
| scuttleGlobalThisExceptions[i] = String(scuttleGlobalThisExceptions[i]) |
There was a problem hiding this comment.
I forgot why, but someone told me
a = `${thingToTurnToString}`
is better. I never verified and don't remember the reasoning though. 🙈
There was a problem hiding this comment.
Guess we'll never know 😱😱😱😱
| continue | ||
| } | ||
| const parts = prop.split('/'); | ||
| extraPropsToAvoid[i] = new RegExp(parts.slice(1, -1).join('/'), parts[parts.length - 1]) |
|
looks good, plz regenerate lavapack runtime |
scuttleGlobalThisExceptionsoptional argument in addition to the already existing string values support.Before:
After:
(both
/HTML[a-zA-Z]*Element/and"/HTML[a-zA-Z]*Element/"will work)Any property that starts with
/will be interpreted as a RE and not as a standard string propertyTo get more context on scuttling feature visit Start compartment globalThis scuttling (browserify only) #360