src,permission: emit dc messages from C++ and use --permission-audit #61869
src,permission: emit dc messages from C++ and use --permission-audit #61869RafaelGSS wants to merge 6 commits intonodejs:mainfrom
Conversation
Add a C++ API for diagnostics channels that allows native code to check for subscribers and publish messages without unnecessary JS boundary crossings. Uses a shared AliasedUint32Array buffer between C++ and JS to track subscriber counts per channel, enabling a fast inline check (HasSubscribers) that reads the buffer directly.
Add --permission-audit flag that enables the permission model in warning-only mode. Instead of throwing ERR_ACCESS_DENIED, it emits a message via diagnostics channel and allows the operation to continue. Publish permission check results to per-scope diagnostics channels (e.g., node:permission-model:fs) so users can observe permission decisions at runtime via diagnostics_channel. Refs: nodejs#59935
|
Review requested:
|
nodejs-github-bot
added
lib / src
RafaelGSS
added
semver-minor
RafaelGSS
added
the
commit-queue-rebase
Qard
left a comment
Qard
left a comment
There was a problem hiding this comment.
Very cool! Not going to block on it, but it would be ideal if we could have a more direct ObjectWrap pattern to link native channels to their JS counterpart and do publishes more directly rather than through this lookup table and callback doing a channel lookup on every publish.
One thing that may need some additional thought either way is what to do if a publish call happens at a point where it's not valid to call into JS at that exact time. What do we do in that situation? Panic? Schedule for next tick?
| dc_binding.setPublishCallback((name, message) => { | ||
| const ch = dc.channel(name); | ||
| if (ch.hasSubscribers) ch.publish(message); | ||
| }); |
There was a problem hiding this comment.
Not a huge fan of needing to call dc.channel(...) within the publish hot-path.
Was there some reason you didn't go for just having the C++ Channel type use ObjectWrap::Wrap(...) to wrap the actual channel object itself? I know we might not be able to do so immediately as the C++ channels might be initialized before we can actually get into JS, but we can late-bind, right?
Perhaps rather than a setPublishCallback we could have something like:
dc_binding.linkNativeChannel((name) => {
return dc.channel(name);
});When that is first called it'd immediately wrap all not-yet-wrapped native channels around their JS equivalent and then store the callback to do so for any future native channels.
There was a problem hiding this comment.
I guess we could check if there are pending when linkNativeChannel is called. Pushing a commit shortly.
The reason I didn't use the ObjectWrap was due to a much larger refactor that would be necessary on diagnostics_channel js, and I was afraid of breaking/slowing the current status quo.
src/permission/permission.cc
Outdated
| diagnostics_channel::Channel ch = | ||
| diagnostics_channel::Channel::Get(env, channel_name); |
There was a problem hiding this comment.
Could we do this ahead of time in Permission::Permission() or something like that to more closely mirror the object-reuse pattern we have on the JS side?
I care less about the perf in this specific case and more thinking about if we are able to support that as a pattern as I would ideally like to start using this to add channels to other parts of native code too. 🤔
|
cc @bengl You may also have opinions on the diagnostics_channel implementation in here. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #61869 +/- ##
==========================================
- Coverage 89.76% 89.71% -0.05%
==========================================
Files 675 677 +2
Lines 204674 204990 +316
Branches 39330 39378 +48
==========================================
+ Hits 183716 183904 +188
- Misses 13235 13301 +66
- Partials 7723 7785 +62
🚀 New features to boost your workflow:
|
RafaelGSS
removed
the
commit-queue-rebase
4 participants
This PR supersedes #60578.
src,permission: add --permission-audit
src: add C++ support for diagnostics channels
PTAL @joyeecheung @Qard