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 78.12500% with 42 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.30%. Comparing base (a876a12) to head (d110091).
⚠️ Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
src/permission/permission.cc 67.34% 19 Missing and 13 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   248314      -11     
  Branches    46861    46862       +1     
==========================================
- Hits       224303   224245      -58     
- Misses      15467    15478      +11     
- Partials     8555     8591      +36     
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/util.cc 87.38% <100.00%> (+0.43%) ⬆️
src/util.h 90.98% <ø> (ø)
src/env.cc 85.08% <91.66%> (-0.27%) ⬇️
src/node_diagnostics_channel.cc 83.60% <71.42%> (+0.09%) ⬆️
... and 3 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.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@jasnell
jasnell force-pushed the jasnell/multiple-permission-improvements branch from 8e7292b to 4c995fc Compare August 9, 2026 16:54
Signed-off-by: James M Snell <jasnell@gmail.com>
@jasnell
jasnell force-pushed the jasnell/multiple-permission-improvements branch from 4c995fc to d110091 Compare August 9, 2026 17:02
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.

4 participants