lib,permission: ignore internalModuleStat on module loading #55797

RafaelGSS · 2024-11-09T01:39:26Z

This improves Permission Model usage when allowing read access to specific 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 attempts to load b, it will fail as it reads $pwd and no permission has been given to this path.

nodejs-github-bot · 2024-11-09T01:39:31Z

Review requested:

@nodejs/loaders
@nodejs/security-wg

codecov · 2024-11-09T03:01:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.40%. Comparing base (a243225) to head (85709c2).
Report is 121 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55797      +/-   ##
==========================================
- Coverage   88.40%   88.40%   -0.01%     
==========================================
  Files         654      654              
  Lines      187819   187809      -10     
  Branches    36139    36130       -9     
==========================================
- Hits       166049   166033      -16     
- Misses      15007    15011       +4     
- Partials     6763     6765       +2

Files with missing lines	Coverage Δ
lib/internal/modules/cjs/loader.js	`97.95% <100.00%> (-0.01%)`	⬇️
src/node_file.cc	`77.39% <ø> (+0.10%)`	⬆️

... and 28 files with indirect coverage changes

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.

nodejs-github-bot · 2024-11-09T15:49:21Z

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

nodejs-github-bot · 2024-11-09T18:10:45Z

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

lib/internal/modules/cjs/loader.js

-    if (insidePath && curPath &&
-      ((permission.isEnabled() && !permission.has('fs.read', curPath)) || _stat(curPath) < 1)
-    ) {
+    if (insidePath && curPath && _stat(curPath) < 1) {


src/node_file.cc

@@ -1037,6 +1037,8 @@ static void ExistsSync(const FunctionCallbackInfo<Value>& args) {
 // Used to speed up module loading.  Returns 0 if the path refers to
 // a file, 1 when it's a directory or < 0 on error (usually -ENOENT.)
 // The speedup comes from not creating thousands of Stat and Error objects.
+// Do not expose this function through public API as it doesn't hold


aduh95 · 2024-11-10T08:15:47Z

test/fixtures/permission/main-module.js

@@ -0,0 +1 @@
+require('./required-module');


Suggested change

require('./required-module');

require('./required-module');

aduh95 · 2024-11-10T08:16:18Z

test/fixtures/permission/main-module.mjs

@@ -0,0 +1 @@
+import './required-module.mjs';


Suggested change

import './required-module.mjs';

import './required-module.mjs';

aduh95 · 2024-11-10T08:16:27Z

test/fixtures/permission/required-module.js

@@ -0,0 +1 @@
+console.log('ok');


Suggested change

console.log('ok');

console.log('ok');

aduh95 · 2024-11-10T08:16:34Z

test/fixtures/permission/required-module.mjs

@@ -0,0 +1 @@
+console.log('ok');


Suggested change

console.log('ok');

console.log('ok');

RafaelGSS · 2024-11-10T12:05:56Z

@aduh95 would you mind if I apply the lint suggestions on fixtures/* in a follow up PR? Otherwise, I will need to spin up a new CI.

nodejs-github-bot · 2024-11-11T17:31:46Z

Landed in 3a0968d

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 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Ulises Gascón <[email protected]>

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 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Ulises Gascón <[email protected]>

ruyadorno · 2024-11-27T07:03:07Z

This commit does not land cleanly on v22.x-staging and will need manual backport in case we want it in v22.x.

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 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Ulises Gascón <[email protected]>

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#56058 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Ulises Gascón <[email protected]>

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. labels Nov 9, 2024

RafaelGSS force-pushed the support-require-module branch from 46a8a65 to b6c7580 Compare November 9, 2024 01:46

avivkeller added the permission Issues and PRs related to the Permission Model label Nov 9, 2024

RafaelGSS added 2 commits November 9, 2024 10:59

fixup! permission: ignore internalModuleStat on module loading

85709c2

RafaelGSS force-pushed the support-require-module branch from 418e0ee to 85709c2 Compare November 9, 2024 14:00

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

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 9, 2024

anonrig approved these changes Nov 9, 2024

View reviewed changes

This comment was marked as abuse.

Sign in to view

CodiumAI-Agent reviewed Nov 10, 2024

View reviewed changes

aduh95 reviewed Nov 10, 2024

View reviewed changes

nodejs blocked CodiumAI-Agent Nov 10, 2024

UlisesGascon approved these changes Nov 11, 2024

View reviewed changes

RafaelGSS added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 11, 2024

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

nodejs-github-bot merged commit 3a0968d into nodejs:main Nov 11, 2024
63 checks passed

RafaelGSS mentioned this pull request Nov 11, 2024

test: fix permission fixtures lint #55819

Merged

RafaelGSS mentioned this pull request Nov 19, 2024

2024-11-20, Version 23.3.0 (Current), @RafaelGSS #55921

Merged

ruyadorno added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label Nov 27, 2024

RafaelGSS mentioned this pull request Nov 28, 2024

[v22.x backport] permission: ignore internalModuleStat on module loading #56058

Merged

RafaelGSS added backport-open-v22.x Indicate that the PR has an open backport and removed backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. labels Nov 28, 2024

ruyadorno mentioned this pull request Dec 20, 2024

2025-01-07, Version 22.13.0 'Jod' (LTS) #56329

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib,permission: ignore internalModuleStat on module loading #55797

lib,permission: ignore internalModuleStat on module loading #55797

RafaelGSS commented Nov 9, 2024

nodejs-github-bot commented Nov 9, 2024

codecov bot commented Nov 9, 2024 •

edited

Loading

nodejs-github-bot commented Nov 9, 2024

nodejs-github-bot commented Nov 9, 2024

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

aduh95 Nov 10, 2024

aduh95 Nov 10, 2024

aduh95 Nov 10, 2024

aduh95 Nov 10, 2024

RafaelGSS commented Nov 10, 2024

nodejs-github-bot commented Nov 11, 2024

ruyadorno commented Nov 27, 2024

	import './required-module.mjs';
	import './required-module.mjs';

lib,permission: ignore internalModuleStat on module loading #55797

lib,permission: ignore internalModuleStat on module loading #55797

Conversation

RafaelGSS commented Nov 9, 2024

nodejs-github-bot commented Nov 9, 2024

codecov bot commented Nov 9, 2024 • edited Loading

Codecov Report

nodejs-github-bot commented Nov 9, 2024

nodejs-github-bot commented Nov 9, 2024

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

aduh95 Nov 10, 2024

Choose a reason for hiding this comment

aduh95 Nov 10, 2024

Choose a reason for hiding this comment

aduh95 Nov 10, 2024

Choose a reason for hiding this comment

aduh95 Nov 10, 2024

Choose a reason for hiding this comment

RafaelGSS commented Nov 10, 2024

nodejs-github-bot commented Nov 11, 2024

ruyadorno commented Nov 27, 2024

codecov bot commented Nov 9, 2024 •

edited

Loading