Skip to content

Commit 0bf67ca

Browse files
committed
vfs: split followup tests into named test files
Replace the monolithic test-vfs-followups.js with individual test files: - test-vfs-overlapping-mounts.js - test-vfs-promises-open.js - test-vfs-stream-properties.js - test-vfs-dir-disposal.js - test-vfs-stats-ino-dev.js - test-vfs-append-write.js - test-vfs-cross-device.js - test-vfs-readdir-symlink-recursive.js - test-vfs-package-json-cache.js - test-vfs-readfile-async.js
1 parent 7346ef3 commit 0bf67ca

19 files changed

Lines changed: 385 additions & 288 deletions

JAMES_RESPONSE.md

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
**Status update on follow-ups from #62328**
2+
3+
Here's a summary of what has been addressed so far and what remains open.
4+
5+
### Addressed
6+
7+
| # | Item | Status |
8+
|---|------|--------|
9+
| **1.1** | VFS bypasses permission model | Addressed — `registerVFS()` throws when `permission.isEnabled()` unless `--allow-fs-vfs` runtime flag is set (added to `src/node_options.cc` in the permission namespace) |
10+
| **1.3** | Overlapping mounts undefined behavior | Addressed — `registerVFS()` detects prefix-overlapping mount points (using `/`-terminated prefix comparison to avoid false positives like `/virtual` vs `/virtual2`) and rejects with `ERR_INVALID_STATE` |
11+
| **1.4** | RealFSProvider symlink jail escape | Addressed — `symlinkSync` validates target stays within root, `readlinkSync` translates paths to VFS-relative, `realpathSync` throws EACCES on escape |
12+
| **2.1** | VirtualFileHandle missing methods | Partially addressed — added `chmod`/`chown`/`utimes`/`datasync`/`sync` (no-op stubs), `readv`/`writev`/`appendFile`, `[Symbol.asyncDispose]()`, `readLines`/`readableWebStream`/`createReadStream`/`createWriteStream` (throw `ERR_METHOD_NOT_IMPLEMENTED`). **Still missing**: `fd` property and `EventEmitter` extension (no `'close'` event) |
13+
| **2.2** | `fsPromises.open()` not intercepted | Addressed — separate `promisesOpen` handler in `setup.js` returns VirtualFileHandle for `fsPromises.open()`, while `open` handler returns numeric FD for callback `fs.open()` |
14+
| **2.3** | Streams missing properties | Addressed — added `bytesRead`/`pending` on VirtualReadStream, `bytesWritten`/`pending` on VirtualWriteStream |
15+
| **2.4** | VirtualDir missing disposal | Addressed — added `[Symbol.asyncDispose]()`, `[Symbol.dispose]()`, `entries()` auto-close via try/finally |
16+
| **2.5** | Stats ino/dev always 0 | Addressed — unique incrementing `ino`, distinctive `dev = 4085` (0xFF9) applied to all three stats factory functions |
17+
| **2.7** | Watcher API mismatches | Partially addressed — `VFSStatWatcher.addListener`/`removeListener` fixed to `(event, listener)` signature. **Still missing**: `VFSWatcher` does not emit `'error'` events from catch blocks |
18+
| **3.1** | Streams stall on destroyed/null fd | Addressed — `_write()` calls `callback(createEBADF('write'))`, `_read()` calls `this.destroy(createEBADF('read'))` |
19+
| **3.2** | `#checkClosed()` always reports 'read' | Addressed — parameterized with `syscall` argument, all call sites pass correct syscall name, both base and subclass `#checkClosed(syscall)` pass it to `createEBADF(syscall)` |
20+
| **3.3** | `writeFileSync` misses 'ax'/'ax+' | Addressed — uses `#isAppend()` instead of manual flag check |
21+
| **3.4** | Dead ternary in `#hookProcessCwd` | Addressed — simplified to single `resolvePath(directory)` |
22+
| **3.5** | SEAProvider statSync reads full asset | Addressed — asset sizes cached in `_assetSizes` map on first access |
23+
| **3.6** | SEAProvider recursive readdir | Addressed — `#readdirRecursive()` walks subdirectories |
24+
| **3.7** | SEAProvider numeric flags | Addressed — `openSync()` handles numeric `0` (O_RDONLY) inline |
25+
| **3.8** | Shared mutable statsArray | Addressed — safety comment added explaining synchronous-copy invariant |
26+
| **5.1** | readFile handler still sync | Partially addressed — `readFile` handler in `setup.js` converted to async (`vfs.promises.readFile`), `lib/fs.js` uses `vfsResult()` to route promise to callback. Other callback handlers still use sync+nextTick |
27+
| **6.2** | Cross-VFS operations | Addressed — `checkSameVFS()` helper validates `rename`/`copyFile`/`link` src and dest resolve to same VFS; throws EXDEV otherwise |
28+
| **6.3** | Package.json cache not invalidated on unmount | Addressed — `deregisterVFS()` calls `clearPackageJSONCache()` exported from `package_json_reader.js` |
29+
| **7.1** | `restoreAll()` error handling | Addressed — each `restore()` wrapped in try/catch, AggregateError thrown after all mocks restored |
30+
| **7.2** | `MockFSContext.vfs` exposes full instance | Addressed — changed to `#vfs` private field with read-only getter |
31+
| **7.3** | Platform-specific root check | Addressed — uses `path.parse(parentDir).root !== parentDir` instead of `!== '/'` |
32+
| **8.1** | MemoryProvider stat size 0 for dynamic files | Addressed — `#createStats` calls `entry.getContentSync().length` when `entry.isDynamic()` |
33+
| **8.2** | MemoryProvider symlinkSync auto-creates dirs | Addressed — changed to `#ensureParent(normalized, false, 'symlink')` consistent with other operations |
34+
| **8.3** | MemoryProvider recursive readdir doesn't follow symlinks | Addressed — `#readdirRecursive` resolves symlinks before checking `isDirectory()` |
35+
| **8.5** | RealFSProvider no watch support | Addressed — `watch()`/`watchFile()`/`unwatchFile()` implemented with path translation, `supportsWatch = true` |
36+
| **10.1** | Watcher unbounded memory growth | Addressed — `kMaxPendingEvents = 1024` cap on `#pendingEvents`, oldest events dropped when full |
37+
| **10.2** | Redundant `#destroyed` in streams | Addressed — removed, uses inherited `this.destroyed` |
38+
| **10.3** | SEAProvider primordials | Addressed — uses `StringPrototypeReplaceAll`, `ArrayFrom`, `StringPrototypeStartsWith` from primordials |
39+
40+
### Tests added
41+
42+
- `test-vfs-overlapping-mounts.js` — overlapping mount rejection, including non-overlapping prefix and root cases (1.3)
43+
- `test-vfs-promises-open.js``fsPromises.open()` interception (2.2)
44+
- `test-vfs-stream-properties.js``bytesRead`/`bytesWritten`/`pending` (2.3)
45+
- `test-vfs-dir-disposal.js``Symbol.asyncDispose`/`Symbol.dispose` (2.4)
46+
- `test-vfs-stats-ino-dev.js` — unique ino, distinctive dev (2.5)
47+
- `test-vfs-append-write.js` — append mode correctness (3.3)
48+
- `test-vfs-cross-device.js` — EXDEV for cross-VFS rename/copyFile/link (6.2)
49+
- `test-vfs-readdir-symlink-recursive.js` — recursive readdir follows symlinks (8.3)
50+
- `test-vfs-package-json-cache.js` — cache cleared on unmount (6.3)
51+
- `test-vfs-readfile-async.js` — readFile uses async handler (5.1)
52+
53+
### Not addressed (requires further work)
54+
55+
| # | Item | Notes |
56+
|---|------|-------|
57+
| **1.2** | Any loaded code can call `mount()` | Design/policy discussion — needs `--experimental-vfs` flag or security event |
58+
| **2.1** (partial) | `fd` property, EventEmitter extension | VirtualFileHandle doesn't expose `fd` numeric property or emit `'close'` event |
59+
| **2.3** (partial) | VirtualWriteStream `close(callback)` | Missing explicit `close(callback)` method |
60+
| **2.7** (partial) | VFSWatcher error events | `VFSWatcher` does not emit `'error'` events from catch blocks in `#getStats()`/`#getStatsFor()` |
61+
| **4.1** | Case-insensitive path comparison (Windows) | Requires Windows-specific path normalization |
62+
| **4.2** | UNC paths unsupported | Windows-specific |
63+
| **4.3** | Virtual CWD hardcoded `/` separator | Windows-specific |
64+
| **4.4** | findVFSPackageJSON mixed separator | Note only |
65+
| **5.1** (partial) | Other callback APIs still sync | Only `readFile` converted; broader pattern remains |
66+
| **5.2** | `existsSync` in async code paths | `findVFSWith()` still uses `existsSync` in paths used by async handlers |
67+
| **5.3** | Linear scan of activeVFSList | Note only, unlikely to matter in practice |
68+
| **6.1** | Maintenance burden of 164+ interception points | Design discussion |
69+
| **6.4** | C++ format coupling in package.json serialization | Note only |
70+
| **6.5** | legacyMainResolve hardcoded extensions | Note only |
71+
| **8.4** | RealFSProvider path traversal via real-fs symlinks | `#resolvePath` validates logical path but doesn't resolve real-fs symlinks before checking |
72+
| **9.1** | Windows tests minimal | Blocked on Windows path handling (4.x items) |
73+
| **9.4** | Permission model interaction untested | Needs subprocess test with `--permission` flag |
74+
| **10.4** | VirtualFileHandle `#checkClosed` duplication | Note only — inherent to JS private fields |
75+
| **10.5** | file_system.js and setup.js are very large | Note only |
76+
| **10.6** | VirtualFD range starts at 10,000 | Note only |

lib/internal/fs/promises.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -643,7 +643,7 @@ async function copyFile(src, dest, mode) {
643643
async function open(path, flags, mode) {
644644
const h = vfsState.handlers;
645645
if (h !== null) {
646-
const result = h.open(path, flags, mode);
646+
const result = h.promisesOpen(path, flags, mode);
647647
if (result !== undefined) return result;
648648
}
649649
path = getValidatedPath(path);

lib/internal/vfs/file_system.js

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,6 @@ const kVirtualCwd = Symbol('kVirtualCwd');
4646
const kVirtualCwdEnabled = Symbol('kVirtualCwdEnabled');
4747
const kOriginalChdir = Symbol('kOriginalChdir');
4848
const kOriginalCwd = Symbol('kOriginalCwd');
49-
const kAllowWithPermissionModel = Symbol('kAllowWithPermissionModel');
5049

5150
// Lazy-loaded VFS setup
5251
let registerVFS;
@@ -109,7 +108,6 @@ class VirtualFileSystem {
109108
this[kVirtualCwd] = null; // Set when chdir() is called
110109
this[kOriginalChdir] = null; // Saved process.chdir
111110
this[kOriginalCwd] = null; // Saved process.cwd
112-
this[kAllowWithPermissionModel] = options.allowWithPermissionModel === true;
113111
}
114112

115113
/**
@@ -162,14 +160,6 @@ class VirtualFileSystem {
162160
return this[kVirtualCwdEnabled];
163161
}
164162

165-
/**
166-
* Returns true if this VFS allows being used with the permission model.
167-
* @returns {boolean}
168-
*/
169-
get allowWithPermissionModel() {
170-
return this[kAllowWithPermissionModel];
171-
}
172-
173163
// ==================== Virtual Working Directory ====================
174164

175165
/**

lib/internal/vfs/providers/memory.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -985,7 +985,7 @@ class MemoryProvider extends VirtualProvider {
985985
}
986986

987987
if (listener) {
988-
watcher.addListener(listener);
988+
watcher.addListener('change', listener);
989989
}
990990

991991
return watcher;
@@ -1005,7 +1005,7 @@ class MemoryProvider extends VirtualProvider {
10051005
}
10061006

10071007
if (listener) {
1008-
watcher.removeListener(listener);
1008+
watcher.removeListener('change', listener);
10091009
} else {
10101010
// Remove all listeners
10111011
watcher.removeAllListeners('change');

lib/internal/vfs/setup.js

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ const { createENOENT, createEXDEV } = require('internal/vfs/errors');
3030
const { getVirtualFd, closeVirtualFd } = require('internal/vfs/fd');
3131
const { vfsState, setVfsHandlers } = require('internal/fs/utils');
3232
const permission = require('internal/process/permission');
33+
const { getOptionValue } = require('internal/options');
3334

3435
/**
3536
* Converts a path argument (string or URL) to a string path.
@@ -72,10 +73,10 @@ let vfsHandlerObj;
7273
* @param {VirtualFileSystem} vfs The VFS instance to register
7374
*/
7475
function registerVFS(vfs) {
75-
if (permission.isEnabled() && !vfs.allowWithPermissionModel) {
76+
if (permission.isEnabled() && !getOptionValue('--allow-fs-vfs')) {
7677
throw new ERR_INVALID_STATE(
7778
'VFS cannot be used when the permission model is enabled. ' +
78-
'Set allowWithPermissionModel: true to override.',
79+
'Use --allow-fs-vfs to allow it.',
7980
);
8081
}
8182
if (ArrayPrototypeIndexOf(activeVFSList, vfs) === -1) {
@@ -85,8 +86,11 @@ function registerVFS(vfs) {
8586
for (let i = 0; i < activeVFSList.length; i++) {
8687
const existingMount = activeVFSList[i].mountPoint;
8788
if (existingMount == null) continue;
88-
if (StringPrototypeStartsWith(newMount, existingMount) ||
89-
StringPrototypeStartsWith(existingMount, newMount)) {
89+
const newPrefix = newMount === '/' ? '/' : newMount + '/';
90+
const existingPrefix = existingMount === '/' ? '/' : existingMount + '/';
91+
if (newMount === existingMount ||
92+
StringPrototypeStartsWith(newMount, existingPrefix) ||
93+
StringPrototypeStartsWith(existingMount, newPrefix)) {
9094
throw new ERR_INVALID_STATE(
9195
`VFS mount '${newMount}' overlaps with existing mount '${existingMount}'`,
9296
);
@@ -884,6 +888,9 @@ function createVfsHandlers() {
884888
chmod: (path, mode) => vfsOp(path, (vfs, n) => vfs.promises.chmod(n, mode).then(() => true)),
885889
utimes: (path, atime, mtime) => vfsOp(path, (vfs, n) => vfs.promises.utimes(n, atime, mtime).then(() => true)),
886890
open(path, flags, mode) {
891+
return vfsOp(path, (vfs, n) => PromiseResolve(vfs.openSync(n, flags, mode)));
892+
},
893+
promisesOpen(path, flags, mode) {
887894
const pathStr = toPathStr(path);
888895
if (pathStr !== null) {
889896
const r = findVFSForPath(pathStr);

src/node_options.cc

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -663,6 +663,12 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
663663
kAllowedInEnvvar,
664664
false,
665665
OptionNamespaces::kPermissionNamespace);
666+
AddOption("--allow-fs-vfs",
667+
"allow use of virtual filesystem when any permissions are set",
668+
&EnvironmentOptions::allow_fs_vfs,
669+
kAllowedInEnvvar,
670+
false,
671+
OptionNamespaces::kPermissionNamespace);
666672
AddOption("--allow-child-process",
667673
"allow use of child process when any permissions are set",
668674
&EnvironmentOptions::allow_child_process,

src/node_options.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,7 @@ class EnvironmentOptions : public Options {
142142
std::vector<std::string> allow_fs_read;
143143
std::vector<std::string> allow_fs_write;
144144
bool allow_addons = false;
145+
bool allow_fs_vfs = false;
145146
bool allow_inspector = false;
146147
bool allow_child_process = false;
147148
bool allow_net = false;
Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
'use strict';
2+
3+
// writeSync in append mode must append, not overwrite.
4+
5+
require('../common');
6+
const assert = require('assert');
7+
const vfs = require('node:vfs');
8+
9+
const myVfs = vfs.create();
10+
myVfs.writeFileSync('/append.txt', 'init');
11+
12+
const fd = myVfs.openSync('/append.txt', 'a');
13+
const buf = Buffer.from(' more');
14+
myVfs.writeSync(fd, buf, 0, buf.length);
15+
myVfs.closeSync(fd);
16+
17+
const content = myVfs.readFileSync('/append.txt', 'utf8');
18+
assert.strictEqual(content, 'init more');
Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
'use strict';
2+
3+
// Cross-VFS rename, copyFile, and link must throw EXDEV.
4+
5+
require('../common');
6+
const assert = require('assert');
7+
const fs = require('fs');
8+
const vfs = require('node:vfs');
9+
10+
const vfs1 = vfs.create();
11+
vfs1.writeFileSync('/src.txt', 'data');
12+
vfs1.mount('/xdev_a');
13+
14+
const vfs2 = vfs.create();
15+
vfs2.mkdirSync('/dest');
16+
vfs2.mount('/xdev_b');
17+
18+
assert.throws(() => fs.renameSync('/xdev_a/src.txt', '/xdev_b/dest/src.txt'), {
19+
code: 'EXDEV',
20+
});
21+
22+
assert.throws(() => fs.copyFileSync('/xdev_a/src.txt', '/xdev_b/dest/src.txt'), {
23+
code: 'EXDEV',
24+
});
25+
26+
assert.throws(() => fs.linkSync('/xdev_a/src.txt', '/xdev_b/dest/link.txt'), {
27+
code: 'EXDEV',
28+
});
29+
30+
vfs2.unmount();
31+
vfs1.unmount();
Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
'use strict';
2+
3+
// VirtualDir must support Symbol.dispose and Symbol.asyncDispose.
4+
5+
const common = require('../common');
6+
const assert = require('assert');
7+
const fs = require('fs');
8+
const vfs = require('node:vfs');
9+
10+
const myVfs = vfs.create();
11+
myVfs.mkdirSync('/dir');
12+
myVfs.writeFileSync('/dir/a.txt', 'a');
13+
myVfs.mount('/mnt_dirdispose');
14+
15+
// Symbol.dispose via closeSync
16+
{
17+
const dir = fs.opendirSync('/mnt_dirdispose/dir');
18+
dir[Symbol.dispose]();
19+
assert.throws(() => dir.closeSync(), { code: 'ERR_DIR_CLOSED' });
20+
}
21+
22+
// Symbol.asyncDispose via close
23+
{
24+
const dir = fs.opendirSync('/mnt_dirdispose/dir');
25+
dir[Symbol.asyncDispose]().then(common.mustCall(() => {
26+
assert.throws(() => dir.closeSync(), { code: 'ERR_DIR_CLOSED' });
27+
}));
28+
}
29+
30+
myVfs.unmount();

0 commit comments

Comments
 (0)