Disallow destructuring internalbinding c

[Ceres6]

follow up of #53947

[nodejs-github-bot]

Review requested:

• @nodejs/crypto
• @nodejs/http
• @nodejs/net

[Ceres6]

I have created the typings for crypto, I don't mind creating the rest, but I wanted to check that I had the correct approach

[anonrig]

Can you make sure all files have an EOL?

[Ceres6]

Done!

[joyeecheung]

Isn't this something that only matter for certain internal bindings that use the binding object as a receiver, and doesn't apply to all binding objects? Disallowing destructing on all other unrelated bindings would lead to a perf cost and for bindings that are public, this allows users to modify them and affect internals.

[joyeecheung]

I think a better way to enforce the fast API dependency on creation context may be - do not use the receiver. Just make that part of the API - always pass the binding object as the first argument. The linter configs that disallow destructing is not strong enough - someone can still do const method = binding.method, and then invoke it without the binding. But if we enforce that it must be called as method(binding, ...), then it'd be safer.

[mcollina]

lgtm

[legendecas]

Destructing symbols and constants should be safe and disallowing this could be way too verbose.

[Ceres6]

@joyeecheung I'm happy to implement your approach, but I'd need some guidance on how to do it and how to identify which bindings need to enforce it

[Ceres6]

@anonrig can you help me unblock this?

[joyeecheung]

I'm happy to implement your approach, but I'd need some guidance on how to do it and how to identify which bindings need to enforce it

Basically, look for all the fast APIs (you can probably easily find them by searching CFunction::Make) and check the arguments of the fast API callbacks - if the first argument is a receiver that actually gets used in the callback, insert a second Local<Object> argument and use it instead, ignoring the first argument in C++, then update all corresponding JS land calls to pass the original receiver as the first JS call argument (which corresponds to the second argument of the C++ fast API callback).

[Ceres6]

@joyeecheung I created a draft PR with the changes for fs.internalModuleStat adding the receiver as second argument. It would be very helpful if you could take a look to see if I'm on the right track before migrating all the rest, as there are bunch of occurrences.

[Ceres6]

Closing in favour of #54408