GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Remove Redundant Path Resolution in Internal File System Utilities by avivkeller · Pull Request #51964 · nodejs/node

Conversation

@avivkeller

@avivkeller avivkeller commented Mar 5, 2024

Copy link
Copy Markdown
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."

  • src,process: initial permission model implementation #44004 introduced this functionality initially when relative paths posed a threat. However, with the current landscape, they are no longer necessary.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@avivkeller
Update utils.js
a652ceb
@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to file-system APIs and the fs module. 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
marco-ippolito
marco-ippolito requested changes Mar 5, 2024
View reviewed changes

@marco-ippolito marco-ippolito left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@avivkeller

avivkeller commented Mar 5, 2024

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@avivkeller avivkeller closed this Mar 5, 2024
@avivkeller
avivkeller 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

Reviewers

@marco-ippolito marco-ippolito marco-ippolito requested changes
@RafaelGSS RafaelGSS Awaiting requested review from RafaelGSS

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@avivkeller @marco-ippolito @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.