util: restrict custom inspect function + vm.Context interaction #33690
Conversation
When `util.inspect()` is called on an object with a custom inspect function, and that object is from a different `vm.Context`, that function will not receive any arguments that access context-specific data anymore.
nodejs-github-bot
added
the
util
addaleax
added
the
author ready
| for (const key of ObjectKeys(ret)) { | ||
| if ((typeof ret[key] === 'object' || typeof ret[key] === 'function') && | ||
| ret[key] !== null) { | ||
| delete ret[key]; |
There was a problem hiding this comment.
What about cloning objects and to set the prototype to the destination's Object prototype? The returned object and stylize function could also be set to the destination's Object and Function prototype.
There was a problem hiding this comment.
I’ve thought about that, but it’s not trivial to determine what the Object prototype is in that case.
There was a problem hiding this comment.
Something like that should work well enough:
let proto = maybeCustom
while(true) {
const next = Object.getPrototypeOf(proto)
if (next === null) {
if (proto === maybeCustom)
proto = null
break
}
proto = next
}There is no guarantee that it's actually the Object prototype but it should be. It would also be possible to check for the constructor name (which is also not "reliable") but I would just use this.
There was a problem hiding this comment.
I would prefer to have consistent behavior here rather than to take a guess at the correct prototype.
There was a problem hiding this comment.
The behavior might actually cause issues inside of the vm context: we return the object prototype in the "regular" context and return a null prototype otherwise. The code inside of the VM might not be aware of that and expect an object as prototype and use e.g., hasOwnProperty. Using this pattern should work in all cases where the user did not actively manipulate the passed through functions prototype and that is very unlikely.
There was a problem hiding this comment.
Right, I understand that problem. I still think consistency is more important than the problems that could occur from mis-guessing the prototype.
| stylized = ctx.stylize(value, flavour); | ||
| } catch {} | ||
|
|
||
| if (typeof stylized !== 'string') return value; |
There was a problem hiding this comment.
Is this required? The current stylize function does not check for strings and I would keep those aligned (especially, since it's not something supported to do).
We should probably only filter objects and functions.
There was a problem hiding this comment.
Isn’t the purpose of .stylize() to transform strings into other strings?
There was a problem hiding this comment.
Yes but this function is not filtering anything in the "regular context". I would try to keep the behavior aligned (and it's not a supported function anyway).
There was a problem hiding this comment.
Yes but this function is not filtering anything in the "regular context". I would try to keep the behavior aligned
I can do this if you feel strongly about it, but imo the function is broken anyway if it does not return a string.
and it's not a supported function anyway.
It’s used in examples in the documentation, so I think it qualifies as officially supported.
There was a problem hiding this comment.
(Also, practically speaking, I don’t think anybody is going to override this function with a custom one.)
There was a problem hiding this comment.
It’s used in examples in the documentation, so I think it qualifies as officially supported.
Thanks for pointing that out (even though there's no description of what it would do). I was not aware of that.
practically speaking, I don’t think anybody is going to override this function with a custom one.
The guard here would never be triggered with the internal stylize function.
the function is broken anyway if it does not return a string.
Absolutely. It would either trigger an error in such cases or it changes the printed output to the stringified value. It would therefore align the behavior more closely by using my suggestion.
There was a problem hiding this comment.
practically speaking, I don’t think anybody is going to override this function with a custom one.
The guard here would never be triggered with the internal stylize function.
See the example above.
the function is broken anyway if it does not return a string.
Absolutely. It would either trigger an error in such cases or it changes the printed output to the stringified value. It would therefore align the behavior more closely by using my suggestion.
I can add stringification here as well, inside the try/catch. If I understand correctly, that is your main concern? I don’t see any issue with that.
There was a problem hiding this comment.
Okay, I’ve implemented that.
|
Landed in 58bae4d |
When `util.inspect()` is called on an object with a custom inspect function, and that object is from a different `vm.Context`, that function will not receive any arguments that access context-specific data anymore. PR-URL: #33690 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
When `util.inspect()` is called on an object with a custom inspect function, and that object is from a different `vm.Context`, that function will not receive any arguments that access context-specific data anymore. PR-URL: #33690 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
When `util.inspect()` is called on an object with a custom inspect function, and that object is from a different `vm.Context`, that function will not receive any arguments that access context-specific data anymore. PR-URL: #33690 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
When `util.inspect()` is called on an object with a custom inspect function, and that object is from a different `vm.Context`, that function will not receive any arguments that access context-specific data anymore. PR-URL: #33690 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
When `util.inspect()` is called on an object with a custom inspect function, and that object is from a different `vm.Context`, that function will not receive any arguments that access context-specific data anymore. PR-URL: #33690 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
When
util.inspect()is called on an object with a custom inspectfunction, and that object is from a different
vm.Context,that function will not receive any arguments that access
context-specific data anymore.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes