-
-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix!: correctly detect if file is outside base path on Windows #59
base: main
Are you sure you want to change the base?
Conversation
if (relativeFilePath.startsWith("..")) { | ||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is already performed in getConfigWithStatus()
.
|
||
//----------------------------------------------------------------------------- | ||
// Helpers | ||
//----------------------------------------------------------------------------- | ||
|
||
// calculate base path using import.meta | ||
const basePath = path.dirname(new URL(import.meta.url).pathname); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Windows, this was giving a path like /c:/foo/bar/rewrite/packages/config-array/tests
, where the c:
is considered a top-level directory rather than a drive specifier. This was producing incorrect results with path.resolve(basePath)
like C:\c:\foo\bar\rewrite\packages\config-array\tests
.
@@ -2860,6 +3010,30 @@ describe("ConfigArray", () => { | |||
); | |||
}); | |||
|
|||
it("should return true when the directory is the parent of the base path", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to ensure that a relative path like ".."
without a slash is recognized as external.
} | ||
|
||
// On non-Windows systems, `toNamespacedPath` returns the argument as is. | ||
export default sep === "\\" ? win32ToNamespacedPath : arg => arg; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem like a safe check. Again, because we aren't going to know what format a string is in when it's passed in a browser environment, I don't think we can assume path.sep
is accurate for the data we'll be getting.
It seems safer to check for the first instance of \
and /
in the string? Something like:
const isWinPath = Boolean(filePath.indexOf("/") - file.Path.indexOf("\\"));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The input file path could be relative, so there may not be enough indication to determine if it's a Unix or a Windows path. Another complication is that path.relative
and path.dirname
have different implementations for Unix and Windows (the path separator /
vs. \
is not the only difference). These functions will return different results depending on the platform where Node.js runs even for the same input.
If ConfigArray
needs to work with both Windows and Unix paths on any platform including browsers, and not depend on cwd
at all, it would be better to avoid using node:path
functions to manipulate paths and implement the whole logic internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If
ConfigArray
needs to work with both Windows and Unix paths on any platform including browsers, and not depend oncwd
at all, it would be better to avoid usingnode:path
functions to manipulate paths and implement the whole logic internally.
This was actually the intent. Originally, the only reference to path
I had was path.relative
, which I intended to replace with an inline function in order to avoid that dependency. My goal going forward was to make sure we weren't starting to use even more of path
to ensure everything would work in browsers correctly.
Whatever we do, I just don't want to add even deeper dependency on the path
module. If we need to rethink things a bit, then let's do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've started working on generic path handling helpers to handle both Unix and Windows style paths regardless of the current platform. I'm planning to show a prototype in this PR to get some feedback.
We may want to consider using the Deno standard path library, which is not runtime or OS specific: |
It looks like that library contains different implementations for windows and posix, and by default, it selects the implementation for the current platform. That is similar to how Node.js It would be useful to find a library that can be installed via npm so we don't need to copy the source files manually. |
|
fc765d2
to
77d7e88
Compare
Finally made it work as expected using only the Deno standard library for path handling and without considering the current directory. This fixes the way external files are matched on Windows by including drive letters or UNC server names when resolving relative paths. An important note: external files are files located outside the base path, but the base path is an optional parameter. If no base path is specified, files will never be considered external. This changes the current behavior where the base path defaults to be the current directory if not specified (in ESLint, the base path is always specified). |
This is actually the way I thought it would work when I first implemented it. 😄 I don't think this will affect users because we always pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. As I mentioned, I think we should mark this as breaking.
Would like @mdjermanovic to review before merging.
@@ -1 +1,2 @@ | |||
package-lock = false | |||
@jsr:registry=https://npm.jsr.io |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would end users need to add this to their .npmrc
files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried using npm pack
-ed version of config-array from this branch in eslint/eslint, and npm install
fails with:
$ npm i
npm ERR! code E404
npm ERR! 404 Not Found - GET https://registry.npmjs.org/@jsr%2fstd__path - Not found
npm ERR! 404
npm ERR! 404 '@jsr/std__path@^1.0.2' is not in this registry.
npm ERR! 404
npm ERR! 404 Note that you can also install from a
npm ERR! 404 tarball, folder, http url, or git url.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can expect users to change their npm settings so that they can download packages from JSR. We could distribute @std/path
in a separate directory inside config-array
and import it from there. Or we could use the URL of the tarball from JSR to specify the dependecy in package.json:
- "@jsr/std__path": "^1.0.2",
+ "@jsr/std__path": "https://npm.jsr.io/~/11/@jsr/std__path/1.0.2.tgz",
Either way, we will lose the magic of the caret ^
to pick newer versions automatically. I checked the JSR docs at https://jsr.io/docs/using-packages but I didn't find any recommendations on how to use their packages with npm apart from registering JSR in .npmrc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can one of you open an issue on JSR for this? I can't imagine they didn't encounter this before, but before we spin our wheels, it would be nice to verify if we're doing things the right way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened a discussion here: jsr-io/jsr#701
I'd say let's just use the URL for now. We can always update the approach if/when they respond to your question. |
The build is now failing because the generated CommonJS module is trying to var posixPath = require('@jsr/std__path/posix');
var windowsPath = require('@jsr/std__path/windows'); I'm not sure why there was no build error in my previous commit. The error doesn't seem connected to the way |
If we're going to bundle |
Thanks! I tried to bundle dist/esm/index.js:316:46 - error TS1313: The body of an 'if' statement cannot be the empty statement.
316 if (lastSlash === i - 1 || dots === 1) ; else if (lastSlash !== i - 1 && dots === 2) {
~
dist/esm/index.js:516:15 - error TS2339: Property 'Deno' does not exist on type 'typeof globalThis'.
516 const { Deno } = globalThis;
~~~~
dist/esm/index.js:1148:5 - error TS2353: Object literal may only specify known properties, and 'extended' does not exist in type '{ globstar?: boolean; }'.
1148 extended,
~~~~~~~~
dist/esm/index.js:1590:46 - error TS1313: The body of an 'if' statement cannot be the empty statement.
1590 if (lastSlash === i - 1 || dots === 1) ; else if (lastSlash !== i - 1 && dots === 2) {
~
dist/esm/index.js:1970:13 - error TS2339: Property 'Deno' does not exist on type 'typeof globalThis'.
1970 const { Deno } = globalThis;
~~~~
dist/esm/index.js:2742:5 - error TS2353: Object literal may only specify known properties, and 'extended' does not exist in type '{ globstar?: boolean; }'.
2742 extended,
~~~~~~~~
Found 6 errors in the same file, starting at: dist/esm/index.js:316 So I decided to bundle dist/esm/types.d.ts
dist/esm/index.js
dist/esm/types.ts
dist/esm/std__path/posix.js
dist/esm/std__path/windows.js
dist/esm/index.d.ts
dist/cjs/index.d.cts
dist/cjs/types.ts
dist/cjs/index.cjs
dist/cjs/std__path/posix.cjs
dist/cjs/std__path/windows.cjs I will update this PR shortly with the new solution, and hopefully the CI build will pass this time. |
To make sure I'm understanding correctly: relative paths will no longer work when passed to |
Correct. |
And again, just for clarity, the |
I see the point. if (typeof Deno?.cwd !== "function") {
throw new TypeError("Resolved a drive-letter-less path without a current working directory (CWD)");
}
path = Deno.cwd();
} else {
if (typeof Deno?.env?.get !== "function" || typeof Deno?.cwd !== "function") {
throw new TypeError("Resolved a relative path without a current working directory (CWD)");
} Which is compiled from this source: https://github.com/denoland/std/blob/f1d3885994953eae8a843fb658952c9a6e6e2001/path/windows/resolve.ts#L36-L48. The error messages "Resolved a drive-letter-less path without a current working directory (CWD)" and "Resolved a relative path without a current working directory (CWD)" is what a user of For a better error message, and also to stay safe in case the |
That seems like a good idea...although how would we end up passing an absolute path when |
I've added logic to throw an error when a path is not absolute in a783ba1. |
In order to keep the current behavior unchanged we could resolve the - const config = configArray.getConfig(filename);
+ const config = configArray.getConfig(path.resolve(filename)); Also, we should manually resolve the function normalizeCwd(cwd) {
if (cwd) {
- return cwd;
+ return path.resolve(cwd);
}
if (typeof process === "object") {
return process.cwd();
} I tried it and with those two changes, all We could also change the way the |
Which file is that first code snippet from? I don't think we want to create a dependency on Do we ever pass a relative |
Both snippets are from linter.js in ESLint:
Actually
On closer look there are only three unit tests where the
We still need a mechanism to resolve relative file paths in |
Ah good point. However, the current usage is limited to chopping up filenames rather than resolving. I don't think we can do
I'm wondering, could we allow relative paths and just |
Yes, in order to use
You mean |
Yes, exactly. And sorry, I meant if config array receives a relative path, it should resolve it against the config array's
Is that true even after we switched to |
Okay, I tried this solution and it seems to work well. In ESLint, only a small change in the Here is what I did:
No. With the changes in this PR, when |
Is that necessary? It seems like this should work without a |
It should be fine to use to Shall I update the PR to make |
I think so. @mdjermanovic what do you think? |
Makes sense to me 👍 |
Thanks for the feedback. I've made |
// select path-handling implementations depending on the base path | ||
this.path = getPathImpl(this.basePath); | ||
|
||
// On Windows, `path.relative()` returns an absolute path when given two paths on different drives. | ||
// The namespaced base path is useful to make sure that calculated relative paths are always relative. | ||
// On Unix, it is identical to the base path. | ||
this.namespacedBasePath = this.path.toNamespacedPath(this.basePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these field be private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think yes. We could make them public in the future if there's a need for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 2495406.
@@ -0,0 +1,28 @@ | |||
export default [ | |||
{ | |||
input: "../../node_modules/@jsr/std__path/posix/mod.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to make this not depend on where the package is installed and also not depend on its internal structure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in dd0d81b.
Prerequisites checklist
What is the purpose of this pull request?
Fix logic that detects when a file is outside the base path so it works well with Windows-style paths. Also fixed an edge case with paths that start with ".." but are not external like "..file.js".
What changes did you make? (Give an overview)
Windows paths have some unique features not shared by Unix paths. For one, absolute local paths start with a drive letter followed by a colon like "C:". The fact that the drive letter must be the first character in a local path means that a relative path cannot specify a drive. In Node.js,
path.relative()
will just return the absolute path of the second argument when it cannot calculate a relative path across different drives, so for example withpath.relative("C:\\dir", "D:\\file.js")
.Besides local paths, Windows uses UNC paths in the form
\\Server\Volume\dir1\dir2\file
to specify files on a network. Those paths are always absolute.Both
config-array
and ESLint use relative paths to check if a file is inside a base directory: if the file's relative path from the directory starts with "..", then the file is not in the directory. This check is currently failing when the base directory and the file were located on different drives or servers, because as previously mentionedpath.relative()
doesn't handle those cases.The fix I've come up with consists in calculating what Node.js calls namespace-prefixed paths of the base directory and the file and calling
path.relative()
on those two. Namespaced paths are prefixed with\\?\
for local paths or with\\?\UNC
for network paths. When the provided arguments are namespaced paths on different drives,path.relative()
will return a path in the form..\..\C:\dir1\dir2\file.js
. While such a path is not valid as it contains a drive letter in middle position, it can be readily recognized as for being outside the base path because of the leading..
and discarded as external.Related Issues
eslint
: https://github.com/eslint/eslint/compare/update-for-rewrite-pr-59Is there anything you'd like reviewers to focus on?