Skip to content
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

Remove Redundant Path Resolution in Internal File System Utilities #51964

Closed
wants to merge 1 commit into from

Conversation

RedYetiDev
Copy link
Member

Summary

This pull request eliminates redundant path resolution within the internal file system (fs) utilities. Previously, this resolution was implemented to prevent path traversal via relative paths. However, with the mitigation of this threat, the code responsible for converting absolute paths has become unnecessary.

Proof of Concept

process.cwd = () => '../'; // Sets CWD to relative
fs.readFileSync('unsafe/file.txt')

Executing this code with --experimental-permission --allow-fs-read=safe will result in Error: Access to this API has been restricted, even though path.resolve returns a relative path. Therefore, the conversion of absolute paths is no longer essential.

Related Issues

"Due to the PrefixRadixTree (fs_permission) lookup, relative paths are not supported. For this reason, the possiblyTransformPath was needed. I do have plans to create a pretty similar path.resolve on the C++ side so the possiblyTransformPath won't be needed, but I'll do it in a second iteration."

A discussion here mentioned this change, but did not end up implementing it. While this code is redundant, I can't locate the original pull-request that made it so.

Note

I am a JS dev, but not a C one. While I know that this all works from the JS side, I am no expert in identifying the C code that makes this redundant, and the commit that added it.

@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 Mar 5, 2024
@MoLow MoLow requested a review from RafaelGSS March 5, 2024 07:49
Copy link
Member

@marco-ippolito marco-ippolito left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, unfortunately this change breaks every single test on fs permission model.
Can you explain why the check is redundant, and the reasoning behind this change?

@RedYetiDev
Copy link
Member Author

My bad, I realized a mistake, see the issue I posted.

@RedYetiDev RedYetiDev closed this Mar 5, 2024
@RedYetiDev RedYetiDev deleted the patch-1 branch March 5, 2024 11:22
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.

None yet

3 participants