FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: apply multiple permissions improvements by jasnell · Pull Request #65158 · nodejs/node · GitHub

/ node Public

src: apply multiple permissions improvements - #65158

Closed
jasnell wants to merge 11 commits into
nodejs:mainfrom
jasnell:jasnell/multiple-permission-improvements
ConversationCommits (11)ChecksFiles changed
Closed

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

Conversation

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
src: cache permission strings
fd56532 
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>
src: use DictionaryTemplate for permission diag channel message
bac8975 
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>
src: make minor cleanup to permission checks
dfb1e00 
Getting the name of the channel is unnecessary.

Signed-off-by: James M Snell <jasnell@gmail.com>
src: simplify c++ diagnostics channel API
85cab29 
Signed-off-by: James M Snell <jasnell@gmail.com>
src: apply multiple general cleanups to permissions
551b23b 
Signed-off-by: James M Snell <jasnell@gmail.com>
src: make permission storage a bit more efficient
1998f95 
Use a fixed array rather than an unordered list

Signed-off-by: James M Snell <jasnell@gmail.com>
src: apply a modest performance perf to permissions
77a3aa4 
Improve the way the RadixTree works and apply a
fast api call.

Signed-off-by: James M Snell <jasnell@gmail.com>
src: simplify includes in permissions
857db71 
Signed-off-by: James M Snell <jasnell@gmail.com>
src: simplify permissions with BooleanPermissions
78bfa4f 
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 requested review from Qard and RafaelGSS August 9, 2026 02:28

nodejs-github-bot commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

Review requested:

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

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 changed the title src: cache permission strings src: apply multiple permissions improvements Aug 9, 2026

This comment was marked as outdated.

Sign in to view

codecov Bot commented Aug 9, 2026 •
edited
Loading

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 (c32ea43) to head (d110091).
⚠️ Report is 84 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      248326   248314      -12     
  Branches    46859    46862       +3     
==========================================
- Hits       224293   224245      -48     
+ Misses      15481    15478       -3     
- Partials     8552     8591      +39
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.42%) ⬇️
src/node_diagnostics_channel.cc 83.60% <71.42%> (+0.09%) ⬆️
... and 3 more

... and 51 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.

src: apply minor namespace format tweak in permissions
7dd8d04 
Signed-off-by: James M Snell <jasnell@gmail.com>
jasnell force-pushed the jasnell/multiple-permission-improvements branch from a6b8717 to 7dd8d04 Compare August 9, 2026 05:12

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view
jasnell force-pushed the jasnell/multiple-permission-improvements branch from 8e7292b to 4c995fc Compare August 9, 2026 16:54
test: add permission fast api test
d110091 
Signed-off-by: James M Snell <jasnell@gmail.com>
jasnell force-pushed the jasnell/multiple-permission-improvements branch from 4c995fc to d110091 Compare August 9, 2026 17:02

nodejs-github-bot commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/75733/

jasnell added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Aug 10, 2026
jasnell added a commit that referenced this pull request Aug 12, 2026
src: apply a modest performance perf to permissions
9e733fa 
Improve the way the RadixTree works and apply a
fast api call.

Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
jasnell added a commit that referenced this pull request Aug 12, 2026
src: simplify includes in permissions
55e00cf 
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
jasnell added a commit that referenced this pull request Aug 12, 2026
src: simplify permissions with BooleanPermissions
d157b00 
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>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
jasnell added a commit that referenced this pull request Aug 12, 2026
src: apply minor namespace format tweak in permissions
dee1fe7 
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
jasnell added a commit that referenced this pull request Aug 12, 2026
test: add permission fast api test
d996610 
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

jasnell commented Aug 12, 2026

Copy link
Copy Markdown
Member Author

Landed in 075d748...d996610

jasnell closed this Aug 12, 2026
aduh95 pushed a commit that referenced this pull request Aug 13, 2026
src: cache permission strings
cd670f4 
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>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
aduh95 pushed a commit that referenced this pull request Aug 13, 2026
src: use DictionaryTemplate for permission diag channel message
4b657bd 
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>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
aduh95 pushed a commit that referenced this pull request Aug 13, 2026
src: make minor cleanup to permission checks
22f7018 
Getting the name of the channel is unnecessary.

Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
aduh95 pushed a commit that referenced this pull request Aug 13, 2026
src: simplify c++ diagnostics channel API
4404d23 
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
aduh95 pushed a commit that referenced this pull request Aug 13, 2026
src: apply multiple general cleanups to permissions
631c160 
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
aduh95 pushed a commit that referenced this pull request Aug 13, 2026
src: make permission storage a bit more efficient
08a0a80 
Use a fixed array rather than an unordered list

Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
aduh95 pushed a commit that referenced this pull request Aug 13, 2026
src: apply a modest performance perf to permissions
d556d3b 
Improve the way the RadixTree works and apply a
fast api call.

Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
aduh95 pushed a commit that referenced this pull request Aug 13, 2026
src: simplify includes in permissions
7447410 
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
aduh95 pushed a commit that referenced this pull request Aug 13, 2026
src: simplify permissions with BooleanPermissions
c549dc2 
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>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
aduh95 pushed a commit that referenced this pull request Aug 13, 2026
src: apply minor namespace format tweak in permissions
96d6b03 
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
aduh95 pushed a commit that referenced this pull request Aug 13, 2026
test: add permission fast api test
6145dda 
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
aduh95 pushed a commit that referenced this pull request Aug 25, 2026
src: cache permission strings
f77aa2f 
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>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
aduh95 pushed a commit that referenced this pull request Aug 25, 2026
src: use DictionaryTemplate for permission diag channel message
968bdd0 
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>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
aduh95 pushed a commit that referenced this pull request Aug 25, 2026
src: make minor cleanup to permission checks
ec8fe27 
Getting the name of the channel is unnecessary.

Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
aduh95 pushed a commit that referenced this pull request Aug 25, 2026
src: simplify c++ diagnostics channel API
ac920e0 
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
aduh95 pushed a commit that referenced this pull request Aug 25, 2026
src: apply multiple general cleanups to permissions
521aaf1 
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
aduh95 pushed a commit that referenced this pull request Aug 25, 2026
src: make permission storage a bit more efficient
efb649e 
Use a fixed array rather than an unordered list

Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
aduh95 pushed a commit that referenced this pull request Aug 25, 2026
src: apply a modest performance perf to permissions
d4ced88 
Improve the way the RadixTree works and apply a
fast api call.

Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
aduh95 pushed a commit that referenced this pull request Aug 25, 2026
src: simplify includes in permissions
5954b13 
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
aduh95 pushed a commit that referenced this pull request Aug 25, 2026
src: simplify permissions with BooleanPermissions
981cfa5 
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>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
aduh95 pushed a commit that referenced this pull request Aug 25, 2026
src: apply minor namespace format tweak in permissions
3a47f1f 
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
aduh95 pushed a commit that referenced this pull request Aug 25, 2026
test: add permission fast api test
bf86fab 
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #65158
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
github-actions Bot mentioned this pull request Aug 25, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Qard Qard approved these changes
meixg meixg approved these changes
RafaelGSS Awaiting requested review from RafaelGSS

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL