nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
104.27k stars 28.06k forks source link

src,permission: resolve path on fs_permission #52761

Closed RafaelGSS closed 2 weeks ago

RafaelGSS commented 2 weeks ago

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

nodejs-github-bot commented 2 weeks ago

Review requested:

github-actions[bot] commented 2 weeks ago
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 commented 2 weeks ago

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

nodejs-github-bot commented 2 weeks ago
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 commented 2 weeks ago

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 commented 2 weeks ago
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 
PR-URL: https://github.com/nodejs/node/pull/52761
Reviewed-By: Marco Ippolito 
Reviewed-By: Rich Trott 
--------------------------------------------------------------------------------
[detached HEAD 93ede0c8c1] src,permission: resolve path on fs_permission
 Author: RafaelGSS 
 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: https://github.com/nodejs/node/pull/52761
Reviewed-By: Marco Ippolito 
Reviewed-By: Rich Trott 
--------------------------------------------------------------------------------
[detached HEAD 2c028a1c18] fixup! remove known limitation mention
 Author: RafaelGSS 
 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 commented 2 weeks ago

Landed in 15456e4e57235834b78d7f1aac5382047d4a83fd