[jazelly]

fs.realpath() and fs.realpathSync() resolved symlink targets with
path.resolve(), which collapses .. before the preceding symlink is
resolved. A link like d -> c/../d was turned back into d, causing
an infinite loop.

Fixed this by resolving .. against the already resolved parent instead.

This matches how native implements realpath:
.. is applied to the already-resolved prefix, and symlink targets are
spliced into the remaining path without lexical normalization.

Fixes: #60295
Signed-off-by: jazelly xzha4350@gmail.com

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.68%. Comparing base (128f247) to head (f72d6d1).
⚠️ Report is 28 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #65457      +/-   ##
==========================================
- Coverage   90.69%   90.68%   -0.02%     
==========================================
  Files         797      797              
  Lines      279611   279630      +19     
  Branches    53787    53800      +13     
==========================================
- Hits       253603   253584      -19     
- Misses      16295    16335      +40     
+ Partials     9713     9711       -2     

Files with missing lines	Coverage Δ
lib/fs.js	97.20% <100.00%> (+0.03%)	⬆️

... and 37 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.

[jazelly]

Hi @nodejs/fs, would appreciate any reviews

[jakecastelli]

I think this change may introduce a permission-model regression in the JS fs.realpathSync() walk.
Repro (current main, run with --permission --allow-fs-read=):
mkdir -p jail secret && echo x > secret/marker && ln -s "$PWD/secret" jail/link
node --permission --allow-fs-read="$PWD/jail" -p "fs.realpathSync('$PWD/jail/link/..')"

On main this returns $PWD/jail, which is inside the grant. With this PR it returns $PWD, which is outside the grant, and does not throw.

toAbsolutePathPreservingDots() keeps .. in the path so that it is applied after the symlink is resolved. The walk then leaves the granted tree, and the lstat/stat/readlink calls made during the walk are not checked against the grant. The permission check on the input string sees the lexical path, which is inside the grant, so it passes.

Expected behaviour: ERR_ACCESS_DENIED, which is what the async fs.realpath() returns on the current main.

Suggestion: check each base against the fs.read grant before it is lstat'd, stat'd, or readlink'd during the walk, and add a regression test under test/parallel/ that covers link/.. with --permission.

[jazelly]

@jakecastelli Thanks for the review!

I can see there are 2 concerns here

1. the permission model when it comes to allow-fs-read with the symlink.
2. correct behaviour for this case, $PWD/jail vs $PWD

For 1, I think this is a known imperfection.

On the current main line, with --allow-fs-read=jail granted, fs.realpathSync('jail/link/marker') already returns $PWD/secret/marker, which is outside the grant.

As for 2. $PWD/jail vs $PWD, this is actually what I am trying to align in this PR.

• the realpathSync.native gives the same result as $PWD
• and the promises.realpath

The js version, realpathSync is currently doing wrong on main, as it operates on string directly by offsetting jail/link/.. to jail/. However, the correct logic should be

1. lstat the symlink on $PWD/jail/link first, and found $PWD/secret/
2. do the .., which goes to parent $PWD

So $PWD should be the correct one.

Hope that makes sense.

[jakecastelli]

yes, my bad, it should be "$PWD", I made a mistake

[nodejs-github-bot]

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

[jakecastelli]

lgtm

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 40aad9e