Skip to content

permission: reuse cached env strings - #64912

Closed
agape1225 wants to merge 1 commit into
nodejs:mainfrom
agape1225:permission-use-cached-strings
Closed

permission: reuse cached env strings#64912
agape1225 wants to merge 1 commit into
nodejs:mainfrom
agape1225:permission-use-cached-strings

Conversation

@agape1225

Copy link
Copy Markdown
Contributor

This PR replaces FIXED_ONE_BYTE_STRING(isolate, "...") literals in
src/permission/permission.cc with the Environment-cached
permission_string()/resource_string() accessors already used
elsewhere in the same file. Follow-up to #59891, which didn't cover
this file.

Permission::is_scope_granted() and Permission::Drop() built the
"permission" and "resource" diagnostics message keys with
FIXED_ONE_BYTE_STRING(isolate, ...) on every publish, even though
Environment already caches these exact strings via
env->permission_string() and env->resource_string()
(src/env_properties.h). One call site in the same file already used
the cached accessor; these two did not, and Environment is already
in scope at both sites.

Assisted-by: Claude Sonnet 5
Signed-off-by: agape1225 <49804691+agape1225@users.noreply.github.com>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/security-wg

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 1, 2026
@codecov

codecov Bot commented Aug 1, 2026

Copy link
Copy Markdown

Codecov Report

βœ… All modified and coverable lines are covered by tests.
βœ… Project coverage is 90.27%. Comparing base (b9dacd4) to head (615612e).
⚠️ Report is 179 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #64912      +/-   ##
==========================================
- Coverage   90.29%   90.27%   -0.03%     
==========================================
  Files         760      760              
  Lines      247061   247061              
  Branches    46585    46587       +2     
==========================================
- Hits       223092   223035      -57     
- Misses      15451    15496      +45     
- Partials     8518     8530      +12     
Files with missing lines Coverage Ξ”
src/permission/permission.cc 82.32% <100.00%> (ΓΈ)

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

leah-1ee added a commit to leah-1ee/node that referenced this pull request Aug 5, 2026
Follow-up to nodejs#59891, covering files it missed.
Overlapping locations (node_ffi.cc, crypto_util.cc) were already
handled by nodejs#64760.

Refs: nodejs#59891
Refs: nodejs#64760
Refs: nodejs#64912

Signed-off-by: leah-1ee <selee3196@gmail.com>
leah-1ee added a commit to leah-1ee/node that referenced this pull request Aug 5, 2026
Follow-up to nodejs#59891, covering files it missed.
Overlapping locations (node_ffi.cc, crypto_util.cc) were already
handled by nodejs#64760.

Refs: nodejs#59891
Refs: nodejs#64760
Refs: nodejs#64912

Signed-off-by: leah-1ee <selee3196@gmail.com>
@daeyeon daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 6, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 6, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

nodejs-github-bot pushed a commit that referenced this pull request Aug 8, 2026
Follow-up to #59891, covering files it missed.
Overlapping locations (node_ffi.cc, crypto_util.cc) were already
handled by #64760.

Refs: #59891
Refs: #64760
Refs: #64912

Signed-off-by: leah-1ee <selee3196@gmail.com>
PR-URL: #65039
Refs: #59891
Refs: #64760
Refs: #64912
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@daeyeon daeyeon added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 10, 2026
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 10, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/64912
βœ”  Done loading data for nodejs/node/pull/64912
----------------------------------- PR info ------------------------------------
Title      permission: reuse cached env strings (#64912)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     agape1225:permission-use-cached-strings -> nodejs:main
Labels     c++, author ready, needs-ci, commit-queue
Commits    1
 - permission: reuse cached env strings
Committers 1
 - agape1225 <49804691+agape1225@users.noreply.github.com>
PR-URL: https://github.com/nodejs/node/pull/64912
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/64912
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
--------------------------------------------------------------------------------
   β„Ή  This PR was created on Sat, 01 Aug 2026 13:16:38 GMT
   βœ”  Approvals: 2
   βœ”  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/64912#pullrequestreview-4838784169
   βœ”  - Daeyeon Jeong (@daeyeon): https://github.com/nodejs/node/pull/64912#pullrequestreview-4876387866
   βœ”  Last GitHub CI successful
   β„Ή  Last Full PR CI on 2026-08-09T11:58:23Z: https://ci.nodejs.org/job/node-test-pull-request/75689/
- Querying data for job/node-test-pull-request/75689/
βœ”  Build data downloaded
   βœ”  Last Jenkins CI successful
--------------------------------------------------------------------------------
   βœ”  No git cherry-pick in progress
   βœ”  No git am in progress
   βœ”  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
   ad3432189c..4a4cc1ed24  main       -> origin/main
βœ”  origin/main is now up-to-date
main is out of sync with origin/main. Mismatched commits:
 - ff1744022d build: deprecate always enabled `--enable-static`
 - 4a4cc1ed24 build: deprecate always enabled `--enable-static`
--------------------------------------------------------------------------------
HEAD is now at 4a4cc1ed24 build: deprecate always enabled `--enable-static`
   βœ”  Reset to origin/main
- Downloading patch for 64912
From https://github.com/nodejs/node
 * branch                  refs/pull/64912/merge -> FETCH_HEAD
βœ”  Fetched commits as 5f1ef0a95376..615612e04c54
--------------------------------------------------------------------------------
Auto-merging src/permission/permission.cc
The previous cherry-pick is now empty, possibly due to conflict resolution.
If you wish to commit it anyway, use:
git commit --allow-empty

Otherwise, please use 'git cherry-pick --skip'
On branch main
Your branch is up to date with 'origin/main'.

You are currently cherry-picking commit 615612e04c.
(all conflicts fixed: run "git cherry-pick --continue")
(use "git cherry-pick --skip" to skip this patch)
(use "git cherry-pick --abort" to cancel the cherry-pick operation)

Untracked files:
(use "git add <file>..." to include in what will be committed)
labels.json
output

nothing added to commit but untracked files present (use "git add" to track)
✘ Failed to apply patches

https://github.com/nodejs/node/actions/runs/31401397043

@aduh95

aduh95 commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Superseded by 41afbd3

@aduh95 aduh95 closed this Aug 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants