This still feels like the wrong abstraction level. What if we did --code-generation-handler ./file.js instead?

A flag won't be usable on many serverless environments, being able to set it at runtime (like this PR proposes) seems more appropriate.

My main concern here is that some random library has no business knowing what i'm passing to eval and new Function. My secondary concern is that the event model is wonkey.

How about this:

const release = v8.setCodeGenerationFromStringHandler((code) => {});
// ...
release();

Once called, it can't be called again until release is called.

This is an overall problem with the events we have on process, even uncaughtException and unhandledRejections have use cases where the application doesn't want modules to attach handlers to it. Kinda outside the scope of this PR, but could we somehow use policies to limit which modules can attach process handlers?

Agreed it would be great to see this kind of stuff moved out into loaders or a similar API as a higher level execution context. We previously discussed disabling high-level permissions on process, I would still like to see us deprecate more and more features from the process global over time. Even a new builtin module import for these features could allow policy scoping for who has access to it.

But however it is cut, likely does seem to require a new API away from process and deprecating the old on process.

@mmarchini do you have a constraint that this needs to be a process event? If not, I think we should go with the single-callback api.

No constraints, as long as the handler can be defined during runtime I'm fine with it.

@devsnek

My main concern here is that some random library has no business knowing what i'm passing to eval and new Function.

That's a very fair point. As of today, almost everything in Node.js can be monkeypatched and leak implementation details. For instance. This is true for core modules, but also for built-in such as RegExp.prototype.exec. eval is the only one I know that is un-mokeypatchable. Most of the time (I'd say 90%), using eval is a bad idea. This helps actually monitor how/why it is used and by who.

I am not sure I understand your secondary concern. This current PR does not change the v8 module.

@vdeturckheim are you saying you want random libraries to be able to tune into all evals and whatnot in the main context?

@devsnek well right now, any library can spy on most of the things other pieces of code do. That's pretty much how APMs work. eval being the main exception to that rule and being at the same time, often something that should be avoided. Providing runtime visibility on calls made to eval to multiple moniroting tool is pretty useful.

@vdeturckheim I can't tell if you're saying you need the event model or not. can you be a bit more direct? afaict the api I proposed above should work.

@devsnek gotcha, sorry I missunderstood the question ^^. I only need to place one callback for my use case. But I am afraid there might be multiple libraries needing it (say, one RASP for security, one enterprise compliance tool and an APM), and a single callback would make the use of them exclusive. That's why I used an event emitter here.

I guess in my ideal world I would like to see this:

setCallback((code) => {
  security(code);
  compliance(code);
  apm(code);
})

do people think this is unworkable?

It would require the end user to set this up and assume no library does it without the end user knowing.
Another solution would be to expose the callback so libraries could composes multiple ones but this would end up witht the same result as the event emitter.

Overall, I found out that in the scope of instrumentation, end users don't really want to know about implementation and vendors often do their own things without considering the other ones really.

Well I won't block on this, but it is certainly not ideal.

@cjihrig thanks for the review, it should be good :)

@nodejs/process

I don't understand the use case for this - why would I use this?

@benjamingr there are multiple reasons why one could want to listen to eval. The one I see and would use is to monitor if some pieces of code use eval and dynamically compare it with inputs (say http request) to ensure there is no exploitable/exploited code injection though this method.

Adding WIP flag as I need to fix a build issue (warning emitted when compiling the callback)

Converting to draft: the error flow is weird, throwing an error in the event listener makes it ignored unless there is certain calls after eval. I need to dig deeper into that before anything can be merged