Skip to content

[v20.x] permission: ignore internalModuleStat on module loading #58185

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: v20.x-staging
Choose a base branch
from

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 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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 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.

3 participants