src,permission: resolve path on fs_permission #52761
Merged
Conversation
This file contains 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
Signed-off-by: RafaelGSS <[email protected]>
|
Review requested:
|
nodejs-github-bot
added
c++
RafaelGSS
force-pushed
the
convert-paths-on-pm
branch
from
2dae42c
to
8e3a422
Compare
github-actions
bot
added
request-ci-failed
Failed to start CI⚠ No approving reviews found ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/8901141681 |
marco-ippolito
approved these changes
May 2, 2024
RafaelGSS
added
author ready
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/52761 ✔ Done loading data for nodejs/node/pull/52761 ----------------------------------- PR info ------------------------------------ Title src,permission: resolve path on fs_permission (#52761) Author Rafael Gonzaga (@RafaelGSS) Branch RafaelGSS:convert-paths-on-pm -> nodejs:main Labels c++, lib / src, author ready, needs-ci, permission Commits 3 - src,permission: resolve path on fs_permission - fixup! remove known limitation mention - fixup! src,permission: resolve path on fs_permission Committers 1 - RafaelGSS PR-URL: https://github.com/nodejs/node/pull/52761 Reviewed-By: Marco Ippolito ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/52761 Reviewed-By: Marco Ippolito -------------------------------------------------------------------------------- ℹ This PR was created on Tue, 30 Apr 2024 15:48:13 GMT ✔ Approvals: 1 ✔ - Marco Ippolito (@marco-ippolito): https://github.com/nodejs/node/pull/52761#pullrequestreview-2036027210 ✘ This PR needs to wait 119 more hours to land (or 0 hours if there is one more approval) ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-05-01T14:44:59Z: https://ci.nodejs.org/job/node-test-pull-request/58839/ - Querying data for job/node-test-pull-request/58839/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/8926738177 |
The feature is experimental and/or deprecated, so breaking changes are permitted anyway, or am I mistaken? |
Trott
approved these changes
May 2, 2024
Trott
added
commit-queue
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/52761 ✔ Done loading data for nodejs/node/pull/52761 ----------------------------------- PR info ------------------------------------ Title src,permission: resolve path on fs_permission (#52761) Author Rafael Gonzaga (@RafaelGSS) Branch RafaelGSS:convert-paths-on-pm -> nodejs:main Labels c++, lib / src, author ready, needs-ci, permission Commits 3 - src,permission: resolve path on fs_permission - fixup! remove known limitation mention - fixup! src,permission: resolve path on fs_permission Committers 1 - RafaelGSS PR-URL: https://github.com/nodejs/node/pull/52761 Reviewed-By: Marco Ippolito Reviewed-By: Rich Trott ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/52761 Reviewed-By: Marco Ippolito Reviewed-By: Rich Trott -------------------------------------------------------------------------------- ℹ This PR was created on Tue, 30 Apr 2024 15:48:13 GMT ✔ Approvals: 2 ✔ - Marco Ippolito (@marco-ippolito): https://github.com/nodejs/node/pull/52761#pullrequestreview-2036027210 ✔ - Rich Trott (@Trott): https://github.com/nodejs/node/pull/52761#pullrequestreview-2037076261 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-05-02T15:52:15Z: https://ci.nodejs.org/job/node-test-pull-request/58839/ - Querying data for job/node-test-pull-request/58839/ ✔ 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 ✔ origin/main is now up-to-date - Downloading patch for 52761 From https://github.com/nodejs/node * branch refs/pull/52761/merge -> FETCH_HEAD ✔ Fetched commits as c5cfdd48497f..8e3a422dfa6b -------------------------------------------------------------------------------- Auto-merging src/node_modules.cc [main 75b0cdb2a9] src,permission: resolve path on fs_permission Author: RafaelGSS Date: Mon Apr 29 16:44:08 2024 -0300 20 files changed, 107 insertions(+), 136 deletions(-) delete mode 100644 test/known_issues/test-permission-model-path-resolution.js [main 90396f1840] fixup! remove known limitation mention Author: RafaelGSS Date: Tue Apr 30 12:49:25 2024 -0300 1 file changed, 2 deletions(-) [main 47716ed756] fixup! src,permission: resolve path on fs_permission Author: RafaelGSS Date: Tue Apr 30 12:52:09 2024 -0300 11 files changed, 31 insertions(+), 63 deletions(-) ✔ Patches applied There are 3 commits in the PR. Attempting autorebase. Rebasing (2/5) Rebasing (3/5)https://github.com/nodejs/node/actions/runs/8931416871 |
RafaelGSS
added
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 15456e4 |
Ch3nYuY
pushed a commit
to Ch3nYuY/node
that referenced
this pull request
May 8, 2024
Signed-off-by: RafaelGSS <[email protected]> PR-URL: nodejs#52761 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Rich Trott <[email protected]>
targos
pushed a commit
that referenced
this pull request
May 8, 2024
Signed-off-by: RafaelGSS <[email protected]> PR-URL: #52761 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This resolves a known limitation of the permission model by using the new PathResolve method from the C++ side. Therefore, we don't need to resolve all paths on
lib/fs.jswhen the pm is enabled, we do it only when checkingis_grantedIt's unlikely to happen, but this might affect users relying on
resource:being always the absolute path. With this change,resourcewill be equal to the given param to fs.* calls.cc: @nodejs/security-wg