src,permission: resolve path on fs_permission #52761

RafaelGSS · 2024-04-30T15:48:13Z

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.js when the pm is enabled, we do it only when checking is_granted

It's unlikely to happen, but this might affect users relying on resource: being always the absolute path. With this change, resource will be equal to the given param to fs.* calls.

cc: @nodejs/security-wg

Signed-off-by: RafaelGSS <[email protected]>

nodejs-github-bot · 2024-04-30T15:48:18Z

Review requested:

@nodejs/security-wg

github-actions · 2024-04-30T20:24:41Z

Failed to start CI

   ⚠  No approving reviews found
   ✘  Refusing to run CI on potentially unsafe PR

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

nodejs-github-bot · 2024-05-01T14:44:59Z

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

nodejs-github-bot · 2024-05-02T15:52:15Z

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/.ncu

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

Trott · 2024-05-02T22:34:45Z

It's unlikely to happen, but this might affect users relying on resource: being always the absolute path. With this change, resource will be equal to the given param to fs.* calls.

The feature is experimental and/or deprecated, so breaking changes are permitted anyway, or am I mistaken?

nodejs-github-bot · 2024-05-02T22:42:31Z

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)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src,permission: resolve path on fs_permission

Signed-off-by: RafaelGSS [email protected]
PR-URL: #52761
Reviewed-By: Marco Ippolito [email protected]
Reviewed-By: Rich Trott [email protected]

[detached HEAD 93ede0c8c1] src,permission: resolve path on fs_permission
Author: RafaelGSS [email protected]
Date: Mon Apr 29 16:44:08 2024 -0300
20 files changed, 68 insertions(+), 129 deletions(-)
delete mode 100644 test/known_issues/test-permission-model-path-resolution.js
Rebasing (4/5)
Rebasing (5/5)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fixup! remove known limitation mention

PR-URL: #52761
Reviewed-By: Marco Ippolito [email protected]
Reviewed-By: Rich Trott [email protected]

[detached HEAD 2c028a1c18] fixup! remove known limitation mention
Author: RafaelGSS [email protected]
Date: Tue Apr 30 12:49:25 2024 -0300
1 file changed, 2 deletions(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

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

nodejs-github-bot · 2024-05-03T03:40:50Z

Landed in 15456e4

Signed-off-by: RafaelGSS <[email protected]> PR-URL: nodejs#52761 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Signed-off-by: RafaelGSS <[email protected]> PR-URL: #52761 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Rich Trott <[email protected]>

marco-ippolito · 2024-06-17T08:28:53Z

If we want to land this on v20 it requires manual backport

Signed-off-by: RafaelGSS <[email protected]> PR-URL: nodejs#52761 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Rich Trott <[email protected]>

src,permission: resolve path on fs_permission

52f470e

Signed-off-by: RafaelGSS <[email protected]>

RafaelGSS added the permission Issues and PRs related to the Permission Model label Apr 30, 2024

RafaelGSS requested a review from tniessen April 30, 2024 15:48

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 Apr 30, 2024

RafaelGSS added 2 commits April 30, 2024 12:49

fixup! remove known limitation mention

5f430a1

fixup! src,permission: resolve path on fs_permission

8e3a422

RafaelGSS force-pushed the convert-paths-on-pm branch from 2dae42c to 8e3a422 Compare April 30, 2024 19:44

RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 30, 2024

github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Apr 30, 2024

RafaelGSS mentioned this pull request Apr 30, 2024

Disable Jenkins CI / git node request CI when no approvals nodejs/build#3696

Open

marco-ippolito approved these changes May 2, 2024

View reviewed changes

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 May 2, 2024

Trott approved these changes May 2, 2024

View reviewed changes

Trott added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels May 2, 2024

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 May 2, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 3, 2024

nodejs-github-bot merged commit 15456e4 into nodejs:main May 3, 2024
75 checks passed

targos pushed a commit that referenced this pull request May 8, 2024

src,permission: resolve path on fs_permission

850ff02

Signed-off-by: RafaelGSS <[email protected]> PR-URL: #52761 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Rich Trott <[email protected]>

targos mentioned this pull request May 13, 2024

v22.2.0 release proposal #52971

Merged

marco-ippolito added the backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. label Jun 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src,permission: resolve path on fs_permission #52761

src,permission: resolve path on fs_permission #52761

RafaelGSS commented Apr 30, 2024 •

edited

Loading

nodejs-github-bot commented Apr 30, 2024

github-actions bot commented Apr 30, 2024

nodejs-github-bot commented May 1, 2024

nodejs-github-bot commented May 2, 2024

Trott commented May 2, 2024

nodejs-github-bot commented May 2, 2024

Signed-off-by: RafaelGSS [email protected]
PR-URL: #52761
Reviewed-By: Marco Ippolito [email protected]
Reviewed-By: Rich Trott [email protected]

PR-URL: #52761
Reviewed-By: Marco Ippolito [email protected]
Reviewed-By: Rich Trott [email protected]

Successfully rebased and updated refs/heads/main.

nodejs-github-bot commented May 3, 2024

marco-ippolito commented Jun 17, 2024

src,permission: resolve path on fs_permission #52761

src,permission: resolve path on fs_permission #52761

Conversation

RafaelGSS commented Apr 30, 2024 • edited Loading

nodejs-github-bot commented Apr 30, 2024

github-actions bot commented Apr 30, 2024

nodejs-github-bot commented May 1, 2024

nodejs-github-bot commented May 2, 2024

Trott commented May 2, 2024

nodejs-github-bot commented May 2, 2024

Signed-off-by: RafaelGSS [email protected] PR-URL: #52761 Reviewed-By: Marco Ippolito [email protected] Reviewed-By: Rich Trott [email protected]

PR-URL: #52761 Reviewed-By: Marco Ippolito [email protected] Reviewed-By: Rich Trott [email protected]

Successfully rebased and updated refs/heads/main.

nodejs-github-bot commented May 3, 2024

marco-ippolito commented Jun 17, 2024

RafaelGSS commented Apr 30, 2024 •

edited

Loading

Signed-off-by: RafaelGSS [email protected]
PR-URL: #52761
Reviewed-By: Marco Ippolito [email protected]
Reviewed-By: Rich Trott [email protected]

PR-URL: #52761
Reviewed-By: Marco Ippolito [email protected]
Reviewed-By: Rich Trott [email protected]