Skip to content

Commit a863a07

Browse files
chenxinyancJounQin
chenxinyanc
and
JounQin
authored
fix: rework on handling DOS device paths on Windows (#84)
Co-authored-by: JounQin <[email protected]>
1 parent de4bbe4 commit a863a07

File tree

6 files changed

+72
-35
lines changed

6 files changed

+72
-35
lines changed

‎src/error.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,8 @@ pub enum ResolveError {
4141
#[error("{0}")]
4242
IOError(IOError),
4343

44-
/// For example, Windows UNC path with Volume GUID is not supported.
44+
/// Indicates the resulting path won't be able consumable by NodeJS `import` or `require`.
45+
/// For example, DOS device path with Volume GUID (`\\?\Volume{...}`) is not supported.
4546
#[error("Path {0:?} contains unsupported construct.")]
4647
PathNotSupported(PathBuf),
4748

‎src/file_system.rs‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ use cfg_if::cfg_if;
77
#[cfg(feature = "yarn_pnp")]
88
use pnp::fs::{LruZipCache, VPath, VPathInfo, ZipCache};
99

10+
use crate::ResolveError;
11+
1012
/// File System abstraction used for `ResolverGeneric`
1113
pub trait FileSystem: Send + Sync {
1214
/// See [std::fs::read]
@@ -54,9 +56,13 @@ pub trait FileSystem: Send + Sync {
5456
/// Returns the resolution of a symbolic link.
5557
///
5658
/// # Errors
59+
/// * Returns an error of [`ResolveError::IOError`] kind if there is an IO error invoking [`std::fs::read_link`].
60+
/// * Returns an error of [`ResolveError::PathNotSupported`] kind if the symlink target cannot be represented
61+
/// as a path that can be consumed by the `import`/`require` syntax of Node.js.
62+
/// Caller should not try to follow the symlink in this case.
5763
///
5864
/// See [std::fs::read_link]
59-
fn read_link(&self, path: &Path) -> io::Result<PathBuf>;
65+
fn read_link(&self, path: &Path) -> Result<PathBuf, ResolveError>;
6066

6167
/// See [std::fs::canonicalize]
6268
///
@@ -186,15 +192,11 @@ impl FileSystemOs {
186192
///
187193
/// See [std::fs::read_link]
188194
#[inline]
189-
pub fn read_link(path: &Path) -> io::Result<PathBuf> {
195+
pub fn read_link(path: &Path) -> Result<PathBuf, ResolveError> {
190196
let path = fs::read_link(path)?;
191197
cfg_if! {
192198
if #[cfg(target_os = "windows")] {
193-
match crate::windows::try_strip_windows_prefix(path) {
194-
// We won't follow the link if we cannot represent its target properly.
195-
Ok(p) | Err(crate::ResolveError::PathNotSupported(p)) => Ok(p),
196-
_ => unreachable!(),
197-
}
199+
crate::windows::strip_windows_prefix(path)
198200
} else {
199201
Ok(path)
200202
}
@@ -258,7 +260,7 @@ impl FileSystem for FileSystemOs {
258260
Self::symlink_metadata(path)
259261
}
260262

261-
fn read_link(&self, path: &Path) -> io::Result<PathBuf> {
263+
fn read_link(&self, path: &Path) -> Result<PathBuf, ResolveError> {
262264
cfg_if! {
263265
if #[cfg(feature = "yarn_pnp")] {
264266
match VPath::from(path)? {
@@ -267,7 +269,7 @@ impl FileSystem for FileSystemOs {
267269
VPath::Native(path) => Self::read_link(&path),
268270
}
269271
} else {
270-
Self::read_link(path)
272+
Self::try_read_link(path)
271273
}
272274
}
273275
}

‎src/fs_cache.rs‎

Lines changed: 28 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ impl<Fs: FileSystem> Cache for FsCache<Fs> {
8080
let path = cached_path.to_path_buf();
8181
cfg_if! {
8282
if #[cfg(target_os = "windows")] {
83-
crate::windows::try_strip_windows_prefix(path)
83+
crate::windows::strip_windows_prefix(path)
8484
} else {
8585
Ok(path)
8686
}
@@ -236,20 +236,34 @@ impl<Fs: FileSystem> FsCache<Fs> {
236236
);
237237

238238
if self.fs.symlink_metadata(path.path()).is_ok_and(|m| m.is_symlink) {
239-
let link = self.fs.read_link(normalized.path())?;
240-
if link.is_absolute() {
241-
return self.canonicalize_impl(&self.value(&link.normalize()));
242-
} else if let Some(dir) = normalized.parent() {
243-
// Symlink is relative `../../foo.js`, use the path directory
244-
// to resolve this symlink.
245-
return self
246-
.canonicalize_impl(&dir.normalize_with(&link, self));
239+
match self.fs.read_link(normalized.path()) {
240+
Ok(link) => {
241+
if link.is_absolute() {
242+
return self
243+
.canonicalize_impl(&self.value(&link.normalize()));
244+
} else if let Some(dir) = normalized.parent() {
245+
// Symlink is relative `../../foo.js`, use the path directory
246+
// to resolve this symlink.
247+
return self.canonicalize_impl(
248+
&dir.normalize_with(&link, self),
249+
);
250+
}
251+
debug_assert!(
252+
false,
253+
"Failed to get path parent for {:?}.",
254+
normalized.path()
255+
);
256+
}
257+
Err(ResolveError::PathNotSupported(_)) => {
258+
// No need to follow symlink if the target path cannot be imported in NodeJS.
259+
// Note that per current implementation, if there is a symlink chain, like
260+
// A --> B --> C
261+
// we won't follow the symlink as long as try_read_link(B) is None,
262+
// regardless of whether C is a valid path for NodeJS. This is a corner case.
263+
// We may need to revisit this in the future.
264+
}
265+
Err(e) => return Err(e),
247266
}
248-
debug_assert!(
249-
false,
250-
"Failed to get path parent for {:?}.",
251-
normalized.path()
252-
);
253267
}
254268

255269
Ok(normalized)

‎src/options.rs‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -150,9 +150,24 @@ pub struct ResolveOptions {
150150
/// Default `[]`
151151
pub roots: Vec<PathBuf>,
152152

153-
/// Whether to resolve symlinks to their symlinked location.
153+
/// Whether to resolve symlinks to their symlinked location, if possible.
154154
/// When enabled, symlinked resources are resolved to their real path, not their symlinked location.
155-
/// Note that this may cause module resolution to fail when using tools that symlink packages (like npm link).
155+
/// Note that this may cause module resolution to fail when using tools that symlink packages (like `npm link`).
156+
///
157+
/// Even if this option has been enabled, the resolver may decide not to follow the symlinks if the target cannot be
158+
/// represented as a valid path for `require` or `import` statements in NodeJS. Specifically, we won't follow the symlink if:
159+
/// 1. On Windows, the symlink is a [Volume mount point](https://learn.microsoft.com/en-us/windows/win32/fileio/volume-mount-points)
160+
/// to a Volume that does not have a drive letter.
161+
/// See: How to [mount a drive in a folder](https://learn.microsoft.com/en-us/windows-server/storage/disk-management/assign-a-mount-point-folder-path-to-a-drive).
162+
/// 2. On Windows, the symlink points to a [DOS device path](https://learn.microsoft.com/en-us/dotnet/standard/io/file-path-formats#dos-device-paths)
163+
/// that cannot be reduced into a [traditional DOS path](https://learn.microsoft.com/en-us/dotnet/standard/io/file-path-formats#traditional-dos-paths).
164+
/// For example, all of the following symlink targets _will not_ be followed:
165+
/// * `\\.\Volume{b75e2c83-0000-0000-0000-602f00000000}\folder\` (Volume GUID)
166+
/// * `\\.\BootPartition\folder\file.ts` (Drive name)
167+
///
168+
/// DOS device path either pointing to a drive with drive letter, or a UNC path, will be simplified and followed, such as
169+
/// * `\\.\D:\path\to\file`: reduced to `D:\path\to\file`;
170+
/// * `\\.\UNC\server\share\path\to\file`: reduced to `\\server\share\path\to\file`.
156171
///
157172
/// Default `true`
158173
pub symlinks: bool,

‎src/tests/memory_fs.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ use std::{
33
path::{Path, PathBuf},
44
};
55

6-
use crate::{FileMetadata, FileSystem};
6+
use crate::{FileMetadata, FileSystem, ResolveError};
77

88
#[derive(Default)]
99
pub struct MemoryFS {
@@ -72,8 +72,8 @@ impl FileSystem for MemoryFS {
7272
self.metadata(path)
7373
}
7474

75-
fn read_link(&self, _path: &Path) -> io::Result<PathBuf> {
76-
Err(io::Error::new(io::ErrorKind::NotFound, "not a symlink"))
75+
fn read_link(&self, _path: &Path) -> Result<PathBuf, ResolveError> {
76+
Err(io::Error::new(io::ErrorKind::NotFound, "not a symlink").into())
7777
}
7878

7979
fn canonicalize(&self, _path: &Path) -> io::Result<PathBuf> {

‎src/windows.rs‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,16 +4,18 @@ use crate::ResolveError;
44

55
/// When applicable, converts a [DOS device path](https://learn.microsoft.com/en-us/dotnet/standard/io/file-path-formats#dos-device-paths)
66
/// to a normal path (usually, "Traditional DOS paths" or "UNC path") that can be consumed by the `import`/`require` syntax of Node.js.
7-
/// Returns `None` if the path cannot be represented as a normal path.
8-
pub fn try_strip_windows_prefix(path: PathBuf) -> Result<PathBuf, ResolveError> {
7+
///
8+
/// # Errors
9+
/// Returns error of [`ResolveError::PathNotSupported`] kind if the path cannot be represented as a normal path.
10+
pub fn strip_windows_prefix(path: PathBuf) -> Result<PathBuf, ResolveError> {
911
// See https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file
1012
let path_bytes = path.as_os_str().as_encoded_bytes();
1113

1214
let path = if let Some(p) =
1315
path_bytes.strip_prefix(br"\\?\UNC\").or_else(|| path_bytes.strip_prefix(br"\\.\UNC\"))
1416
{
1517
// UNC paths
16-
// SAFETY:
18+
// SAFETY: `as_encoded_bytes` ensures `p` is valid path bytes
1719
unsafe {
1820
PathBuf::from(std::ffi::OsStr::from_encoded_bytes_unchecked(&[br"\\", p].concat()))
1921
}
@@ -29,7 +31,7 @@ pub fn try_strip_windows_prefix(path: PathBuf) -> Result<PathBuf, ResolveError>
2931
// This can happen if the path points to a Mounted Volume without a drive letter.
3032
return Err(ResolveError::PathNotSupported(path));
3133
}
32-
// SAFETY:
34+
// SAFETY: `as_encoded_bytes` ensures `p` is valid path bytes
3335
unsafe { PathBuf::from(std::ffi::OsStr::from_encoded_bytes_unchecked(p)) }
3436
} else {
3537
path
@@ -41,13 +43,16 @@ pub fn try_strip_windows_prefix(path: PathBuf) -> Result<PathBuf, ResolveError>
4143
#[test]
4244
fn test_try_strip_windows_prefix() {
4345
let pass = [
46+
(r"C:\Users\user\Documents\", r"C:\Users\user\Documents\"),
47+
(r"C:\Users\user\Documents\file1.txt", r"C:\Users\user\Documents\file1.txt"),
48+
(r"\\?\C:\Users\user\Documents\", r"C:\Users\user\Documents\"),
4449
(r"\\?\C:\Users\user\Documents\file1.txt", r"C:\Users\user\Documents\file1.txt"),
4550
(r"\\.\C:\Users\user\Documents\file2.txt", r"C:\Users\user\Documents\file2.txt"),
4651
(r"\\?\UNC\server\share\file3.txt", r"\\server\share\file3.txt"),
4752
];
4853

4954
for (path, expected) in pass {
50-
assert_eq!(try_strip_windows_prefix(PathBuf::from(path)), Ok(PathBuf::from(expected)));
55+
assert_eq!(strip_windows_prefix(PathBuf::from(path)), Ok(PathBuf::from(expected)));
5156
}
5257

5358
let fail = [
@@ -57,7 +62,7 @@ fn test_try_strip_windows_prefix() {
5762

5863
for path in fail {
5964
assert_eq!(
60-
try_strip_windows_prefix(PathBuf::from(path)),
65+
strip_windows_prefix(PathBuf::from(path)),
6166
Err(crate::ResolveError::PathNotSupported(PathBuf::from(path)))
6267
);
6368
}

0 commit comments

Comments
 (0)