Skip to content

fs: fix globSync traverse on allowed directory#61552

Open
artsiom-malakhau wants to merge 1 commit intonodejs:mainfrom
artsiom-malakhau:fix-glob-sync-permission-model
Open

fs: fix globSync traverse on allowed directory#61552
artsiom-malakhau wants to merge 1 commit intonodejs:mainfrom
artsiom-malakhau:fix-glob-sync-permission-model

Conversation

@artsiom-malakhau
Copy link
Contributor

Fix an issue where fs.globSync failed to find files when read access was granted only for a certain directory via the --allow-fs-read flag.

Fixes: #61499

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Jan 27, 2026
Fix an issue where fs.globSync failed to find files when read access
was granted only for a certain directory via the --allow-fs-read flag.

Fixes: nodejs#61499
@artsiom-malakhau artsiom-malakhau force-pushed the fix-glob-sync-permission-model branch from 94fba7f to 04e88bd Compare January 27, 2026 19:48
@artsiom-malakhau
Copy link
Contributor Author

artsiom-malakhau commented Jan 27, 2026

Also, while researching the issue, I discovered one thing: globSync uses lstatSync, which contains a check for the presence of permissions

image

At the same time, in the asynch version of glob, lstat from fs/promises.js is used, which does not have this check. That may be strange. I'm still exploring the project, so I cannot say for sure whether this should be fixed. According to the git change history, perhaps @RafaelGSS can give some advice

image

If a fix is indeed needed, I would be glad to work on it

@codecov
Copy link

codecov bot commented Jan 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.78%. Comparing base (e155415) to head (04e88bd).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #61552   +/-   ##
=======================================
  Coverage   89.77%   89.78%           
=======================================
  Files         672      672           
  Lines      203755   203756    +1     
  Branches    39167    39167           
=======================================
+ Hits       182922   182938   +16     
- Misses      13164    13167    +3     
+ Partials     7669     7651   -18     
Files with missing lines Coverage Δ
lib/internal/fs/glob.js 91.78% <100.00%> (+0.37%) ⬆️

... and 34 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@RafaelGSS
Copy link
Member

I'm going to check it as soon as possible.

@RafaelGSS
Copy link
Member

At the same time, in the asynch version of glob, lstat from fs/promises.js is used, which does not have this check. That may be strange. I'm still exploring the project, so I cannot say for sure whether this should be fixed. According to the git change history, perhaps @RafaelGSS can give some advice

This will be fixed soon. Please, feel free to ignore the permission check for globSync then (add an commented test please).

@artsiom-malakhau
Copy link
Contributor Author

Okay, I see, but I’m not sure I understand which test needs to be added. The test for the problem of this issue itself is already part of this PR.

@RafaelGSS
Copy link
Member

RafaelGSS commented Feb 3, 2026

Okay, I see, but I’m not sure I understand which test needs to be added. The test for the problem of this issue itself is already part of this PR.

Feel free to ignore the permission model test I asked for. If possible add a TODO(rafaelgss) add globSync permission model check

@louiellan
Copy link

louiellan commented Feb 3, 2026

I think he might be referring to me about the test cause i was asked to do permission model test and we have similar profile.

@artsiom-malakhau
Copy link
Contributor Author

Okay, I see. So you will also add the comment that Rafael mentioned?

I think he might be referring to me about the test cause i was asked to do permission model test and we have similar profile.

@louiellan
Copy link

Yep, right after this PR lands since I'll be writing on the test file you created, thanks for the fix ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fs.globSync can't traverse on allowed directory by specific --allow-fs-read

4 participants