Skip to content

Conversation

RafaelGSS
Copy link
Member

@RafaelGSS RafaelGSS commented May 5, 2025

This improves Permission Model usage when allowing read access to specifi modules. To achieve that, the permission model check on internalModuleStat has been removed meaning that on module loading, uv_fs_stat is performed on files and folders even when the permission model is enabled. Although a uv_fs_stat is performed, reading/executing the module will still pass by the permission model check.

Without this PR when an app tries to --allow-fs-read=./a.js --allow-fs-read=./b.js where a attempt to load b, it will fails as it reads $pwd and no permission has been given to this path.

PR-URL: #55797
Backport-PR-URL: #58185
Reviewed-By: Yagiz Nizipli [email protected]
Reviewed-By: Ulises Gascón [email protected]

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/security-wg

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. v20.x Issues that can be reproduced on v20.x or PRs targeting the v20.x-staging branch. labels May 5, 2025
RafaelGSS added a commit to RafaelGSS/node that referenced this pull request May 5, 2025
This improves Permission Model usage when allowing read access to
specifi modules. To achieve that, the permission model check on
internalModuleStat has been removed meaning that on module loading,
uv_fs_stat is performed on files and folders even when the permission
model is enabled. Although a uv_fs_stat is performed, reading/executing
the module will still pass by the permission model check.

Without this PR when an app tries to --allow-fs-read=./a.js
--allow-fs-read=./b.js where `a` attempt to load b, it will fails as
it reads $pwd and no permission has been given to this path.

PR-URL: nodejs#55797
Backport-PR-URL: nodejs#58185
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Ulises Gascón <[email protected]>
@RafaelGSS RafaelGSS force-pushed the backport-55797-to-v20 branch from bc3caae to 2a206d3 Compare May 5, 2025 19:42
RafaelGSS added a commit to RafaelGSS/node that referenced this pull request May 5, 2025
This improves Permission Model usage when allowing read access to
specifi modules. To achieve that, the permission model check on
internalModuleStat has been removed meaning that on module loading,
uv_fs_stat is performed on files and folders even when the permission
model is enabled. Although a uv_fs_stat is performed, reading/executing
the module will still pass by the permission model check.

Without this PR when an app tries to --allow-fs-read=./a.js
--allow-fs-read=./b.js where `a` attempt to load b, it will fails as
it reads $pwd and no permission has been given to this path.

PR-URL: nodejs#55797
Backport-PR-URL: nodejs#58185
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Ulises Gascón <[email protected]>
@RafaelGSS RafaelGSS force-pushed the backport-55797-to-v20 branch from 2a206d3 to 994eb5d Compare May 5, 2025 19:45
Copy link
Member

@juanarbol juanarbol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

RafaelGSS added a commit to RafaelGSS/node that referenced this pull request May 5, 2025
This improves Permission Model usage when allowing read access to
specifi modules. To achieve that, the permission model check on
internalModuleStat has been removed meaning that on module loading,
uv_fs_stat is performed on files and folders even when the permission
model is enabled. Although a uv_fs_stat is performed, reading/executing
the module will still pass by the permission model check.

Without this PR when an app tries to --allow-fs-read=./a.js
--allow-fs-read=./b.js where `a` attempt to load b, it will fails as
it reads $pwd and no permission has been given to this path.

PR-URL: nodejs#55797
Backport-PR-URL: nodejs#58185
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Ulises Gascón <[email protected]>
@RafaelGSS RafaelGSS force-pushed the backport-55797-to-v20 branch from 994eb5d to f5c7741 Compare May 5, 2025 22:30
@marco-ippolito marco-ippolito force-pushed the backport-55797-to-v20 branch from f5c7741 to b07d444 Compare June 3, 2025 13:32
marco-ippolito pushed a commit to RafaelGSS/node that referenced this pull request Jun 3, 2025
This improves Permission Model usage when allowing read access to
specifi modules. To achieve that, the permission model check on
internalModuleStat has been removed meaning that on module loading,
uv_fs_stat is performed on files and folders even when the permission
model is enabled. Although a uv_fs_stat is performed, reading/executing
the module will still pass by the permission model check.

Without this PR when an app tries to --allow-fs-read=./a.js
--allow-fs-read=./b.js where `a` attempt to load b, it will fails as
it reads $pwd and no permission has been given to this path.

PR-URL: nodejs#55797
Backport-PR-URL: nodejs#58185
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Ulises Gascón <[email protected]>
@marco-ippolito marco-ippolito force-pushed the v20.x-staging branch 3 times, most recently from ed0cf8c to 2b3b2a0 Compare June 11, 2025 07:21
This improves Permission Model usage when allowing read access to
specifi modules. To achieve that, the permission model check on
internalModuleStat has been removed meaning that on module loading,
uv_fs_stat is performed on files and folders even when the permission
model is enabled. Although a uv_fs_stat is performed, reading/executing
the module will still pass by the permission model check.

Without this PR when an app tries to --allow-fs-read=./a.js
--allow-fs-read=./b.js where `a` attempt to load b, it will fails as
it reads $pwd and no permission has been given to this path.

PR-URL: nodejs#55797
Backport-PR-URL: nodejs#58185
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Ulises Gascón <[email protected]>
Signed-off-by: RafaelGSS <[email protected]>
@RafaelGSS RafaelGSS force-pushed the backport-55797-to-v20 branch from b07d444 to e8eaced Compare July 10, 2025 22:39
@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 11, 2025
@github-actions 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 Jul 15, 2025
Copy link
Contributor

Failed to start CI
   ⚠  Commits were pushed since the last approving review:
   ⚠  - permission: ignore internalModuleStat on module loading
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/16306740577

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@RafaelGSS RafaelGSS added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Jul 28, 2025
marco-ippolito pushed a commit that referenced this pull request Aug 14, 2025
This improves Permission Model usage when allowing read access to
specifi modules. To achieve that, the permission model check on
internalModuleStat has been removed meaning that on module loading,
uv_fs_stat is performed on files and folders even when the permission
model is enabled. Although a uv_fs_stat is performed, reading/executing
the module will still pass by the permission model check.

Without this PR when an app tries to --allow-fs-read=./a.js
--allow-fs-read=./b.js where `a` attempt to load b, it will fails as
it reads $pwd and no permission has been given to this path.

PR-URL: #55797
Backport-PR-URL: #58185
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Ulises Gascón <[email protected]>
Signed-off-by: RafaelGSS <[email protected]>
@marco-ippolito
Copy link
Member

Landed in 7f8d1b2

marco-ippolito pushed a commit that referenced this pull request Aug 25, 2025
This improves Permission Model usage when allowing read access to
specifi modules. To achieve that, the permission model check on
internalModuleStat has been removed meaning that on module loading,
uv_fs_stat is performed on files and folders even when the permission
model is enabled. Although a uv_fs_stat is performed, reading/executing
the module will still pass by the permission model check.

Without this PR when an app tries to --allow-fs-read=./a.js
--allow-fs-read=./b.js where `a` attempt to load b, it will fails as
it reads $pwd and no permission has been given to this path.

PR-URL: #55797
Backport-PR-URL: #58185
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Ulises Gascón <[email protected]>
Signed-off-by: RafaelGSS <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Aug 26, 2025
This improves Permission Model usage when allowing read access to
specifi modules. To achieve that, the permission model check on
internalModuleStat has been removed meaning that on module loading,
uv_fs_stat is performed on files and folders even when the permission
model is enabled. Although a uv_fs_stat is performed, reading/executing
the module will still pass by the permission model check.

Without this PR when an app tries to --allow-fs-read=./a.js
--allow-fs-read=./b.js where `a` attempt to load b, it will fails as
it reads $pwd and no permission has been given to this path.

PR-URL: #55797
Backport-PR-URL: #58185
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Ulises Gascón <[email protected]>
Signed-off-by: RafaelGSS <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Aug 27, 2025
This improves Permission Model usage when allowing read access to
specifi modules. To achieve that, the permission model check on
internalModuleStat has been removed meaning that on module loading,
uv_fs_stat is performed on files and folders even when the permission
model is enabled. Although a uv_fs_stat is performed, reading/executing
the module will still pass by the permission model check.

Without this PR when an app tries to --allow-fs-read=./a.js
--allow-fs-read=./b.js where `a` attempt to load b, it will fails as
it reads $pwd and no permission has been given to this path.

PR-URL: #55797
Backport-PR-URL: #58185
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Ulises Gascón <[email protected]>
Signed-off-by: RafaelGSS <[email protected]>
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++. fs Issues and PRs related to the fs subsystem / file system. module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. v20.x Issues that can be reproduced on v20.x or PRs targeting the v20.x-staging branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants