fs: resolve symlinks before parent components

The JavaScript implementation of `fs.realpath()` and
`fs.realpathSync()` joined a symlink target with the rest of the path
through `path.resolve()`, which collapses `..` lexically before the
preceding components have been resolved. A link such as
`d -> c/../d`, where `c` is itself a symlink, was therefore rewritten
back to `d` and the walk never terminated.

Keep `.` and `..` in the path while walking it, and apply `..` to the
already resolved parent, matching `realpath(3)` and
`fs.realpathSync.native()`. Only normalize the final result, and skip
caching link targets containing `..`, whose resolution depends on the
path walked before them.

Thsi is a POSIX only fix. Windows is unaffected.

Signed-off-by: jazelly <xzha4350@gmail.com>
PR-URL: #65457
Fixes: #60295
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>

Author: jazelly

lib/fs.js

@@ -3339,6 +3339,25 @@ if (isWindows) {
   };
 }
 
+// realpath(3) applies `..` to the resolved parent, so on POSIX `.` and `..`
+// are kept in the path while walking it. Windows applies `..` lexically, both
+// in the path being opened and in symlink targets, so the path and the link
+// targets are normalized up front there, matching `fs.realpathSync.native()`.
+function toRealpathInput(p) {
+  if (isWindows) return pathModule.resolve(p);
+  return pathModule.isAbsolute(p) ? p : `${pathModule.resolve()}/${p}`;
+}
+
+function joinRealpathLink(previous, linkTarget) {
+  if (isWindows) return pathModule.resolve(previous, linkTarget);
+  return pathModule.isAbsolute(linkTarget) ? linkTarget : `${previous}${linkTarget}`;
+}
+
+function joinRealpathRemainder(resolvedLink, remainder) {
+  if (isWindows) return pathModule.resolve(resolvedLink, remainder);
+  return remainder.length === 0 ? resolvedLink : `${resolvedLink}/${remainder}`;
+}
+
 /**
  * Returns the resolved pathname.
  * @param {string | Buffer | URL} p
@@ -3357,7 +3376,7 @@ function realpathSync(p, options) {
     p += '';
   }
   validatePath(p);
-  p = pathModule.resolve(p);
+  p = toRealpathInput(p);
 
   // Check the resolved input path up front so that the lstat walk below cannot
   // be used to probe the existence of paths outside the granted set.
@@ -3371,14 +3390,14 @@ function realpathSync(p, options) {
   }
 
   const cache = options[realpathCacheKey];
-  const maybeCachedResult = cache?.get(p);
+  const original = p;
+  const maybeCachedResult = cache?.get(original);
   if (maybeCachedResult) {
     return maybeCachedResult;
   }
 
   const seenLinks = new SafeMap();
   const knownHard = new SafeSet();
-  const original = p;
   // Whether the symlink this walk resolved last pointed at a pipe or a
   // socket, which is where the walk stops. It cannot be read back from the
   // shared stat buffer, which holds the last stat made anywhere in the
@@ -3414,6 +3433,9 @@ function realpathSync(p, options) {
   while (pos < p.length) {
     // find the next part
     const result = nextPart(p, pos);
+    const part = result === -1 ?
+      StringPrototypeSlice(p, pos) :
+      StringPrototypeSlice(p, pos, result);
     previous = current;
     if (result === -1) {
       const last = StringPrototypeSlice(p, pos);
@@ -3426,6 +3448,19 @@ function realpathSync(p, options) {
       pos = result + 1;
     }
 
+    if (part === '' || part === '.') {
+      current = previous;
+      continue;
+    }
+    if (part === '..') {
+      base = pathModule.dirname(previous);
+      current = base;
+      if (current[current.length - 1] !== CHAR_FORWARD_SLASH &&
+          current[current.length - 1] !== CHAR_BACKWARD_SLASH) {
+        current += pathModule.sep;
+      }
+      continue;
+    }
     // Continue if not a symlink, break if a pipe/socket
     if (knownHard.has(base) || cache?.get(base) === base) {
       if (reachedPipeOrSocket) {
@@ -3471,14 +3506,18 @@ function realpathSync(p, options) {
           isFileType(targetStats, S_IFSOCK);
         linkTarget = binding.readlink(base, undefined);
       }
-      resolvedLink = pathModule.resolve(previous, linkTarget);
+      resolvedLink = joinRealpathLink(previous, linkTarget);
 
-      cache?.set(base, resolvedLink);
+      // On POSIX, a target containing `..` resolves relative to the path
+      // walked before it, so it cannot be cached by the link alone.
+      if (isWindows || StringPrototypeIndexOf(linkTarget, '..') === -1) {
+        cache?.set(base, pathModule.resolve(resolvedLink));
+      }
       if (!isWindows) seenLinks.set(id, linkTarget);
     }
 
     // Resolve the link, then start over
-    p = pathModule.resolve(resolvedLink, StringPrototypeSlice(p, pos));
+    p = joinRealpathRemainder(resolvedLink, StringPrototypeSlice(p, pos));
 
     // Skip over roots
     current = base = splitRoot(p);
@@ -3495,6 +3534,7 @@ function realpathSync(p, options) {
     }
   }
 
+  p = pathModule.resolve(p);
   cache?.set(original, p);
   return encodeRealpathResult(p, options);
 }
@@ -3547,7 +3587,7 @@ function realpath(p, options, callback) {
     p += '';
   }
   validatePath(p);
-  p = pathModule.resolve(p);
+  p = toRealpathInput(p);
 
   const seenLinks = new SafeMap();
   const knownHard = new SafeSet();
@@ -3585,11 +3625,14 @@ function realpath(p, options, callback) {
   function LOOP() {
     // Stop if scanned past end of path
     if (pos >= p.length) {
-      return callback(null, encodeRealpathResult(p, options));
+      return callback(null, encodeRealpathResult(pathModule.resolve(p), options));
     }
 
     // find the next part
     const result = nextPart(p, pos);
+    const part = result === -1 ?
+      StringPrototypeSlice(p, pos) :
+      StringPrototypeSlice(p, pos, result);
     previous = current;
     if (result === -1) {
       const last = StringPrototypeSlice(p, pos);
@@ -3602,10 +3645,23 @@ function realpath(p, options, callback) {
       pos = result + 1;
     }
 
+    if (part === '' || part === '.') {
+      current = previous;
+      return process.nextTick(LOOP);
+    }
+    if (part === '..') {
+      base = pathModule.dirname(previous);
+      current = base;
+      if (current[current.length - 1] !== CHAR_FORWARD_SLASH &&
+          current[current.length - 1] !== CHAR_BACKWARD_SLASH) {
+        current += pathModule.sep;
+      }
+      return process.nextTick(LOOP);
+    }
     // Continue if not a symlink, break if a pipe/socket
     if (knownHard.has(base)) {
       if (reachedPipeOrSocket) {
-        return callback(null, encodeRealpathResult(p, options));
+        return callback(null, encodeRealpathResult(pathModule.resolve(p), options));
       }
       return process.nextTick(LOOP);
     }
@@ -3649,12 +3705,12 @@ function realpath(p, options, callback) {
   function gotTarget(err, target) {
     if (err) return callback(err);
 
-    gotResolvedLink(pathModule.resolve(previous, target));
+    gotResolvedLink(joinRealpathLink(previous, target));
   }
 
   function gotResolvedLink(resolvedLink) {
     // Resolve the link, then start over
-    p = pathModule.resolve(resolvedLink, StringPrototypeSlice(p, pos));
+    p = joinRealpathRemainder(resolvedLink, StringPrototypeSlice(p, pos));
     current = base = splitRoot(p);
     pos = current.length;
 

test/parallel/test-fs-realpath-symlink-dot-dot.js

@@ -0,0 +1,108 @@
+// Flags: --expose-internals
+
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const { spawnSync } = require('child_process');
+const fs = require('fs');
+const { realpathCacheKey } = require('internal/fs/utils');
+const path = require('path');
+const tmpdir = require('../common/tmpdir');
+
+if (process.argv[2] === 'child') {
+  if (process.argv[4] === 'callback') {
+    fs.realpath(process.argv[3], common.mustSucceed((result) => {
+      process.stdout.write(result);
+    }));
+  } else {
+    process.stdout.write(fs.realpathSync(process.argv[3]));
+  }
+} else {
+  // Windows applies `..` in symlink targets lexically, so `d -> c\..\d` points
+  // at itself there and the walk fails with ELOOP, like the operating system.
+  if (common.isWindows)
+    common.skip('symlink targets are normalized lexically on Windows');
+  if (!common.canCreateSymLink())
+    common.skip('insufficient privileges');
+
+  tmpdir.refresh();
+
+  // Reproduce a symlink target containing `..` after another symlink:
+  //   c -> a/b/c
+  //   d -> c/../d -> a/b/d
+  // The cache fixture also creates a separate d to ensure that c/../d stays
+  // distinct from the lexically normalized path d.
+  const target = tmpdir.resolve('a', 'b', 'd');
+  fs.mkdirSync(tmpdir.resolve('a', 'b', 'c'), { recursive: true });
+  fs.mkdirSync(target);
+  fs.symlinkSync(path.join('a', 'b', 'c'), tmpdir.resolve('c'), 'dir');
+  fs.symlinkSync(`c${path.sep}..${path.sep}d`, tmpdir.resolve('d'), 'dir');
+
+  const cacheRoot = tmpdir.resolve('cache');
+  const cacheTarget = path.join(cacheRoot, 'a', 'b', 'd');
+  const cachePlain = path.join(cacheRoot, 'd');
+  fs.mkdirSync(path.join(cacheRoot, 'a', 'b', 'c'), { recursive: true });
+  fs.mkdirSync(cacheTarget);
+  fs.mkdirSync(cachePlain);
+  fs.symlinkSync(path.join('a', 'b', 'c'), path.join(cacheRoot, 'c'), 'dir');
+  const cacheDotted = `${cacheRoot}${path.sep}c${path.sep}..${path.sep}d`;
+  const cacheExpected = fs.realpathSync(cacheTarget);
+  const plainExpected = fs.realpathSync(cachePlain);
+
+  function assertCacheIsolation(first, firstExpected, second, secondExpected) {
+    const cache = new Map();
+    const options = { [realpathCacheKey]: cache };
+    assert.strictEqual(fs.realpathSync(first, options), firstExpected);
+    assert.strictEqual(fs.realpathSync(second, options), secondExpected);
+  }
+
+  assertCacheIsolation(cacheDotted, cacheExpected, cachePlain, plainExpected);
+  assertCacheIsolation(cachePlain, plainExpected, cacheDotted, cacheExpected);
+
+  // Run realpathSync in a child because the regression does not return.
+  const result = spawnSync(process.execPath, [
+    '--expose-internals',
+    __filename,
+    'child',
+    tmpdir.resolve('d'),
+  ], {
+    encoding: 'utf8',
+    timeout: common.platformTimeout(5000),
+  });
+
+  assert.ifError(result.error);
+  assert.strictEqual(result.status, 0, result.stderr);
+  assert.strictEqual(result.stdout, target);
+
+  const directInput = `c${path.sep}..${path.sep}d`;
+  const directResult = spawnSync(process.execPath, [
+    '--expose-internals',
+    __filename,
+    'child',
+    directInput,
+  ], {
+    encoding: 'utf8',
+    cwd: tmpdir.path,
+    timeout: common.platformTimeout(5000),
+  });
+
+  assert.ifError(directResult.error);
+  assert.strictEqual(directResult.status, 0, directResult.stderr);
+  assert.strictEqual(directResult.stdout, target);
+
+  const callbackResult = spawnSync(process.execPath, [
+    '--expose-internals',
+    __filename,
+    'child',
+    tmpdir.resolve('d'),
+    'callback',
+  ], {
+    encoding: 'utf8',
+    timeout: common.platformTimeout(5000),
+  });
+
+  assert.ifError(callbackResult.error);
+  assert.strictEqual(callbackResult.status, 0, callbackResult.stderr);
+  assert.strictEqual(callbackResult.stdout, target);
+}