FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fs: pass symlink type in cp when filter is provided · nodejs/node@6305a74 · GitHub

/ node Public

Commit 6305a74

Browse files
shulaoda
authored and
aduh95
committed
fs: pass symlink type in cp when filter is provided
When `fs.cp`/`fs.cpSync` is called with both `verbatimSymlinks: true` and a `filter` function, directory symlinks were incorrectly created as file symlinks on Windows. Without a `filter`, cp takes the C++ fast path (`cpSyncCopyDir`) which uses `std::filesystem::copy_symlink()` and preserves the symlink type automatically. With a `filter`, the JS fallback calls `symlinkSync`/ `symlink` without a `type` argument. On Windows, that causes the type to be auto-detected by stat-ing the resolved target at the destination, but during a recursive copy the target directory may not exist yet at the destination (e.g. `linked/` is copied before `packages/` in alphabetical order). The stat fails and `type` falls back to `'file'`, producing a file symlink in place of a directory symlink. Detect the symlink type from the source (which always exists) via `internalModuleStat(src)` and pass it explicitly to the `symlinkSync`/ `symlink` call sites. `onLink` already computed `srcIsDir` for subdirectory validation; hoist that computation above the early-return paths and thread the derived `symlinkType` through `copyLink` as well. Both the sync (`cp-sync.js`) and async (`cp.js`) implementations are fixed. Add two regression tests that copy a tree containing a relative directory symlink with `verbatimSymlinks: true` and a `filter` function, then assert the destination link still resolves as a directory. Fixes: #62653 Signed-off-by: shulaoda <165626830+shulaoda@users.noreply.github.com> PR-URL: #62654 Reviewed-By: Aviv Keller <me@aviv.sh> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Stefan Stojanovic <stefan.stojanovic@janeasystems.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent c5cb6bc commit 6305a74

4 files changed

Lines changed: 106 additions & 13 deletions

File tree

‎lib/internal/fs/cp/cp-sync.js‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -191,8 +191,10 @@ function onLink(destStat, src, dest, verbatimSymlinks) {
191191
if (!verbatimSymlinks && !isAbsolute(resolvedSrc)) {
192192
resolvedSrc = resolve(dirname(src), resolvedSrc);
193193
}
194+
const srcIsDir = fsBinding.internalModuleStat(src) === 1;
195+
const symlinkType = srcIsDir ? 'dir' : 'file';
194196
if (!destStat) {
195-
return symlinkSync(resolvedSrc, dest);
197+
return symlinkSync(resolvedSrc, dest, symlinkType);
196198
}
197199
let resolvedDest;
198200
try {
@@ -202,14 +204,13 @@ function onLink(destStat, src, dest, verbatimSymlinks) {
202204
// Windows may throw UNKNOWN error. If dest already exists,
203205
// fs throws error anyway, so no need to guard against it here.
204206
if (err.code === 'EINVAL' || err.code === 'UNKNOWN') {
205-
return symlinkSync(resolvedSrc, dest);
207+
return symlinkSync(resolvedSrc, dest, symlinkType);
206208
}
207209
throw err;
208210
}
209211
if (!isAbsolute(resolvedDest)) {
210212
resolvedDest = resolve(dirname(dest), resolvedDest);
211213
}
212-
const srcIsDir = fsBinding.internalModuleStat(src) === 1;
213214

214215
if (srcIsDir && isSrcSubdir(resolvedSrc, resolvedDest)) {
215216
throw new ERR_FS_CP_EINVAL({
@@ -233,12 +234,12 @@ function onLink(destStat, src, dest, verbatimSymlinks) {
233234
code: 'EINVAL',
234235
});
235236
}
236-
return copyLink(resolvedSrc, dest);
237+
return copyLink(resolvedSrc, dest, symlinkType);
237238
}
238239

239-
function copyLink(resolvedSrc, dest) {
240+
function copyLink(resolvedSrc, dest, symlinkType) {
240241
unlinkSync(dest);
241-
return symlinkSync(resolvedSrc, dest);
242+
return symlinkSync(resolvedSrc, dest, symlinkType);
242243
}
243244

244245
module.exports = { cpSyncFn };

‎lib/internal/fs/cp/cp.js‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -336,8 +336,10 @@ async function onLink(destStat, src, dest, opts) {
336336
if (!opts.verbatimSymlinks && !isAbsolute(resolvedSrc)) {
337337
resolvedSrc = resolve(dirname(src), resolvedSrc);
338338
}
339+
const srcIsDir = fsBinding.internalModuleStat(src) === 1;
340+
const symlinkType = srcIsDir ? 'dir' : 'file';
339341
if (!destStat) {
340-
return symlink(resolvedSrc, dest);
342+
return symlink(resolvedSrc, dest, symlinkType);
341343
}
342344
let resolvedDest;
343345
try {
@@ -347,16 +349,14 @@ async function onLink(destStat, src, dest, opts) {
347349
// Windows may throw UNKNOWN error. If dest already exists,
348350
// fs throws error anyway, so no need to guard against it here.
349351
if (err.code === 'EINVAL' || err.code === 'UNKNOWN') {
350-
return symlink(resolvedSrc, dest);
352+
return symlink(resolvedSrc, dest, symlinkType);
351353
}
352354
throw err;
353355
}
354356
if (!isAbsolute(resolvedDest)) {
355357
resolvedDest = resolve(dirname(dest), resolvedDest);
356358
}
357359

358-
const srcIsDir = fsBinding.internalModuleStat(src) === 1;
359-
360360
if (srcIsDir && isSrcSubdir(resolvedSrc, resolvedDest)) {
361361
throw new ERR_FS_CP_EINVAL({
362362
message: `cannot copy ${resolvedSrc} to a subdirectory of self ` +
@@ -380,12 +380,12 @@ async function onLink(destStat, src, dest, opts) {
380380
code: 'EINVAL',
381381
});
382382
}
383-
return copyLink(resolvedSrc, dest);
383+
return copyLink(resolvedSrc, dest, symlinkType);
384384
}
385385

386-
async function copyLink(resolvedSrc, dest) {
386+
async function copyLink(resolvedSrc, dest, symlinkType) {
387387
await unlink(dest);
388-
return symlink(resolvedSrc, dest);
388+
return symlink(resolvedSrc, dest, symlinkType);
389389
}
390390

391391
module.exports = {

‎test/parallel/test-fs-cp-async-verbatim-dir-symlinks-with-filter.mjs‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
// This tests that cp with verbatimSymlinks and filter preserves
2+
// the directory symlink type on Windows (does not create a file symlink).
3+
import { mustNotMutateObjectDeep, isWindows } from '../common/index.mjs';
4+
import { nextdir } from '../common/fs.js';
5+
import assert from 'node:assert';
6+
import {
7+
mkdirSync,
8+
writeFileSync,
9+
symlinkSync,
10+
readlinkSync,
11+
readdirSync,
12+
statSync,
13+
} from 'node:fs';
14+
import { cp } from 'node:fs/promises';
15+
import { join } from 'node:path';
16+
17+
import tmpdir from '../common/tmpdir.js';
18+
tmpdir.refresh();
19+
20+
// Setup source with a relative directory symlink
21+
const src = nextdir();
22+
mkdirSync(join(src, 'packages', 'my-lib'), mustNotMutateObjectDeep({ recursive: true }));
23+
writeFileSync(join(src, 'packages', 'my-lib', 'index.js'), 'module.exports = "hello"');
24+
mkdirSync(join(src, 'linked'), mustNotMutateObjectDeep({ recursive: true }));
25+
symlinkSync(join('..', 'packages', 'my-lib'), join(src, 'linked', 'my-lib'), 'dir');
26+
27+
// Copy with verbatimSymlinks: true AND a filter function
28+
const dest = nextdir();
29+
await cp(src, dest, mustNotMutateObjectDeep({
30+
recursive: true,
31+
verbatimSymlinks: true,
32+
filter: () => true,
33+
}));
34+
35+
// Verify the symlink target is preserved verbatim
36+
const link = readlinkSync(join(dest, 'linked', 'my-lib'));
37+
if (isWindows) {
38+
assert.strictEqual(link.toLowerCase(), join('..', 'packages', 'my-lib').toLowerCase());
39+
} else {
40+
assert.strictEqual(link, join('..', 'packages', 'my-lib'));
41+
}
42+
43+
// Verify the symlink works as a directory (not a file symlink)
44+
const destSymlink = join(dest, 'linked', 'my-lib');
45+
assert.ok(statSync(destSymlink).isDirectory(), 'symlink target should be accessible as a directory');
46+
assert.deepStrictEqual(readdirSync(destSymlink), ['index.js']);

‎test/parallel/test-fs-cp-sync-verbatim-dir-symlinks-with-filter.mjs‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
// This tests that cpSync with verbatimSymlinks and filter preserves
2+
// the directory symlink type on Windows (does not create a file symlink).
3+
import { mustNotMutateObjectDeep, isWindows } from '../common/index.mjs';
4+
import { nextdir } from '../common/fs.js';
5+
import assert from 'node:assert';
6+
import {
7+
cpSync,
8+
mkdirSync,
9+
writeFileSync,
10+
symlinkSync,
11+
readlinkSync,
12+
readdirSync,
13+
statSync,
14+
} from 'node:fs';
15+
import { join } from 'node:path';
16+
17+
import tmpdir from '../common/tmpdir.js';
18+
tmpdir.refresh();
19+
20+
// Setup source with a relative directory symlink
21+
const src = nextdir();
22+
mkdirSync(join(src, 'packages', 'my-lib'), mustNotMutateObjectDeep({ recursive: true }));
23+
writeFileSync(join(src, 'packages', 'my-lib', 'index.js'), 'module.exports = "hello"');
24+
mkdirSync(join(src, 'linked'), mustNotMutateObjectDeep({ recursive: true }));
25+
symlinkSync(join('..', 'packages', 'my-lib'), join(src, 'linked', 'my-lib'), 'dir');
26+
27+
// Copy with verbatimSymlinks: true AND a filter function
28+
const dest = nextdir();
29+
cpSync(src, dest, mustNotMutateObjectDeep({
30+
recursive: true,
31+
verbatimSymlinks: true,
32+
filter: () => true,
33+
}));
34+
35+
// Verify the symlink target is preserved verbatim
36+
const link = readlinkSync(join(dest, 'linked', 'my-lib'));
37+
if (isWindows) {
38+
assert.strictEqual(link.toLowerCase(), join('..', 'packages', 'my-lib').toLowerCase());
39+
} else {
40+
assert.strictEqual(link, join('..', 'packages', 'my-lib'));
41+
}
42+
43+
// Verify the symlink works as a directory (not a file symlink)
44+
const destSymlink = join(dest, 'linked', 'my-lib');
45+
assert.ok(statSync(destSymlink).isDirectory(), 'symlink target should be accessible as a directory');
46+
assert.deepStrictEqual(readdirSync(destSymlink), ['index.js']);

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL