Skip to content

src: apply multiple permissions improvements - #65158

Open
jasnell wants to merge 11 commits into
nodejs:mainfrom
jasnell:jasnell/multiple-permission-improvements
Open

src: apply multiple permissions improvements#65158
jasnell wants to merge 11 commits into
nodejs:mainfrom
jasnell:jasnell/multiple-permission-improvements

Conversation

@jasnell

@jasnell jasnell commented Aug 9, 2026

Copy link
Copy Markdown
Member

Apply a range of improvements to src/permissions/*

  • Avoid recreating common strings
  • Use a DictionaryTemplate for permissions-related Diagnostics Channel messages
  • Simplify a C++ Diagnostics Channel API used in permissions
  • Make permissions storage more efficient (avoiding unsorted map)
  • Use a v8 fast api and radix tree improvement to make permissions checks ~2.5% faster
  • Simplify the permissions structure to eliminate duplication

Signed-off-by: James M Snell jasnell@gmail.com

jasnell added 9 commits August 8, 2026 16:33
Use env_property strings for permissions since those
are fixed. Avoid creating new string instances each
time.

Also use ToV8Value for a couple since we're in here.

Signed-off-by: James M Snell <jasnell@gmail.com>
Since DiagnosticChannel permission messages always have the
same shape and should be as low cost as possible, use a
cached DictionaryTemplate for creating them

Signed-off-by: James M Snell <jasnell@gmail.com>
Getting the name of the channel is unnecessary.

Signed-off-by: James M Snell <jasnell@gmail.com>
Signed-off-by: James M Snell <jasnell@gmail.com>
Signed-off-by: James M Snell <jasnell@gmail.com>
Use a fixed array rather than an unordered list

Signed-off-by: James M Snell <jasnell@gmail.com>
Improve the way the RadixTree works and apply a
fast api call.

Signed-off-by: James M Snell <jasnell@gmail.com>
Signed-off-by: James M Snell <jasnell@gmail.com>
Most of the PermissionBase subclasses used the identical
simple pattern. Rather than define a bunch of individual
identical permissions, use a single utility definition.
Special cases like FsPermission are still possible but
the simple case is kept... well, simple.

Signed-off-by: James M Snell <jasnell@gmail.com>
@jasnell
jasnell requested review from Qard and RafaelGSS August 9, 2026 02:28
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/security-wg

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 9, 2026
@jasnell jasnell changed the title src: cache permission strings src: apply multiple permissions improvements Aug 9, 2026
@nodejs-github-bot

This comment was marked as outdated.

@codecov

codecov Bot commented Aug 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 81.98758% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.30%. Comparing base (a876a12) to head (8e7292b).
⚠️ Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
src/permission/permission.cc 76.82% 5 Missing and 14 partials ⚠️
src/permission/fs_permission.h 88.88% 2 Missing and 2 partials ⚠️
src/permission/fs_permission.cc 70.00% 3 Missing ⚠️
src/node_diagnostics_channel.cc 71.42% 2 Missing ⚠️
src/env.cc 91.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65158      +/-   ##
==========================================
- Coverage   90.32%   90.30%   -0.02%     
==========================================
  Files         759      750       -9     
  Lines      248325   248297      -28     
  Branches    46861    46870       +9     
==========================================
- Hits       224303   224234      -69     
- Misses      15467    15482      +15     
- Partials     8555     8581      +26     
Files with missing lines Coverage Δ
src/env-inl.h 95.01% <100.00%> (+0.02%) ⬆️
src/env.h 98.21% <ø> (ø)
src/node_diagnostics_channel.h 57.14% <ø> (ø)
src/permission/boolean_permission.h 100.00% <100.00%> (ø)
src/permission/permission.h 100.00% <ø> (ø)
src/permission/permission_base.h 100.00% <100.00%> (ø)
src/env.cc 85.39% <91.66%> (+0.04%) ⬆️
src/node_diagnostics_channel.cc 83.60% <71.42%> (+0.09%) ⬆️
src/permission/fs_permission.cc 69.81% <70.00%> (-0.56%) ⬇️
src/permission/fs_permission.h 90.80% <88.88%> (-0.98%) ⬇️
... and 1 more

... and 37 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: James M Snell <jasnell@gmail.com>
@jasnell
jasnell force-pushed the jasnell/multiple-permission-improvements branch from a6b8717 to 7dd8d04 Compare August 9, 2026 05:12
@nodejs-github-bot

This comment was marked as outdated.

Signed-off-by: James M Snell <jasnell@gmail.com>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants