fix!: correctly detect if file is outside base path on Windows #59

fasttime · 2024-06-17T10:50:38Z

Prerequisites checklist

I have read the contributing guidelines.

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 with path.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 mentioned path.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

refs Bug: files on a different drive are not ignored on Windows eslint#18575
preliminary changes in eslint: https://github.com/eslint/eslint/compare/update-for-rewrite-pr-59

Is there anything you'd like reviewers to focus on?

eslint.config.js

fasttime · 2024-06-17T10:58:17Z

packages/config-array/src/config-array.js

-	if (relativeFilePath.startsWith("..")) {
-		return true;
-	}


This check is already performed in getConfigWithStatus().

fasttime · 2024-06-17T11:09:22Z

packages/config-array/tests/config-array.test.js


 //-----------------------------------------------------------------------------
 // Helpers
 //-----------------------------------------------------------------------------

 // calculate base path using import.meta
-const basePath = path.dirname(new URL(import.meta.url).pathname);


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.

fasttime · 2024-06-17T13:00:15Z

packages/config-array/tests/config-array.test.js

@@ -2860,6 +3010,30 @@ describe("ConfigArray", () => {
 				);
 			});

+			it("should return true when the directory is the parent of the base path", () => {


This is to ensure that a relative path like ".." without a slash is recognized as external.

packages/config-array/src/config-array.js

packages/config-array/src/to-namespaced-path.js

nzakas · 2024-06-27T18:11:20Z

packages/config-array/src/to-namespaced-path.js

+}
+
+// On non-Windows systems, `toNamespacedPath` returns the argument as is.
+export default sep === "\\" ? win32ToNamespacedPath : arg => arg;


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("\\"));

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.

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.

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.

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.

nzakas · 2024-08-08T18:06:29Z

We may want to consider using the Deno standard path library, which is not runtime or OS specific:
https://jsr.io/@std/path

fasttime · 2024-08-13T13:27:29Z

We may want to consider using the Deno standard path library, which is not runtime or OS specific: https://jsr.io/@std/path

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 node:path works.

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.

nzakas · 2024-08-13T15:06:51Z

npx jsr add @std/path works. 😄

fasttime · 2024-08-19T08:14:44Z

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).

nzakas · 2024-08-20T14:38:32Z

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 basePath, but we may want to mark this as breaking for this package.

nzakas

LGTM. As I mentioned, I think we should mark this as breaking.

Would like @mdjermanovic to review before merging.

mdjermanovic · 2024-08-25T13:01:42Z

.npmrc

@@ -1 +1,2 @@
 package-lock = false
+@jsr:registry=https://npm.jsr.io


Would end users need to add this to their .npmrc files?

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.

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.

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.

I opened a discussion here: jsr-io/jsr#701

nzakas · 2024-09-03T14:57:08Z

I'd say let's just use the URL for now. We can always update the approach if/when they respond to your question.

fasttime · 2024-09-04T04:28:37Z

The build is now failing because the generated CommonJS module is trying to require() @jsr/std__path, which is an ESM-only package. In packages/config-array/dist/cjs/index.cjs I can see:

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 @jsr/std__path is declared in package.json. Maybe we need to bundle @jsr/std__path in the CommonJS module so it gets transpiled.

mdjermanovic · 2024-09-04T09:37:28Z

Maybe we need to bundle @jsr/std__path in the CommonJS module so it gets transpiled.

If we're going to bundle @jsr/std__path in CJS, then we could bundle it in ESM as well, to avoid having a URL depedency (which I think is not a common practice for packages published to npm).

fasttime · 2024-09-08T13:11:52Z

Maybe we need to bundle @jsr/std__path in the CommonJS module so it gets transpiled.

If we're going to bundle @jsr/std__path in CJS, then we could bundle it in ESM as well, to avoid having a URL depedency (which I think is not a common practice for packages published to npm).

Thanks! I tried to bundle @jsr/std__path in dist/cjs/index.cjs and dist/esm/index.js, but then tsc started to complain because of the checkJs option in tsconfig.json:

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 @jsr/std__path/posix and @jsr/std__path/windows in separate modules that tsc would not check. This seems to be working well and results in the following files being generated:

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.

Co-authored-by: Milos Djermanovic <[email protected]>

nzakas

LGTM. Would like @mdjermanovic to verify before merging.

mdjermanovic · 2024-10-22T05:49:54Z

LGTM. Would like @mdjermanovic to verify before merging.

#59 (comment) still needs to be addressed by disallowing empty string?

fasttime · 2024-10-23T06:17:56Z

#59 (comment) still needs to be addressed by disallowing empty string?

I pushed a commit to disallow empty strings: 4ed3c1c. Also updated the readme file to reflect the changes: 8c30779.

nzakas · 2024-10-23T14:55:39Z

packages/config-array/src/config-array.js

@@ -606,7 +606,7 @@ export class ConfigArray extends Array {
 		 * @property basePath
 		 * @type {string}
 		 */
-		this.basePath = basePath || "/";
+		this.basePath = basePath;


Can we validate here so we can throw a more specific error, like "basePath must be a non-empty string"?

Done in fb712b1, thanks.

mdjermanovic · 2024-10-24T14:06:51Z

packages/config-array/tests/config-array.test.js

-				const filename = path.resolve(basePath, "foo.js");
+				const filename = "foo.js";


After this change, most of the tests will be with relative paths?

nzakas

LGTM pending @mdjermanovic's comment.

mdjermanovic

LGTM, thanks! We can revisit testing at some later point.

* fix: update for eslint/rewrite#59 * revert unnecessary changes with `basePath` defaulting to `"/"` * update to `config-array` v0.19.x * add tests for `Linter` and `RuleTester` * add tests for `lintFile` on a different drive * remove unused fallback logic for relative paths * add a test for `isPathIgnored` on a different drive * add more tests for `lintFiles` * add a test for `lintText` * remove unused param from `Linter#verify` calls * added comments to `lintFiles` test hooks

eslint-github-bot bot added the bug Something isn't working label Jun 17, 2024

fasttime changed the title ~~fix: correctly detect is file is outside base path on Windows~~ fix: correctly detect if file is outside base path on Windows Jun 17, 2024

fasttime commented Jun 17, 2024

View reviewed changes

eslint.config.js Outdated Show resolved Hide resolved

fasttime commented Jun 17, 2024

View reviewed changes

nzakas requested changes Jun 17, 2024

View reviewed changes

packages/config-array/src/config-array.js Outdated Show resolved Hide resolved

packages/config-array/src/config-array.js Outdated Show resolved Hide resolved

packages/config-array/src/config-array.js Outdated Show resolved Hide resolved

fasttime commented Jun 23, 2024

View reviewed changes

packages/config-array/src/config-array.js Outdated Show resolved Hide resolved

fasttime marked this pull request as ready for review June 23, 2024 20:18

nzakas reviewed Jun 24, 2024

View reviewed changes

packages/config-array/src/config-array.js Outdated Show resolved Hide resolved

packages/config-array/src/config-array.js Outdated Show resolved Hide resolved

nzakas reviewed Jun 26, 2024

View reviewed changes

packages/config-array/src/to-namespaced-path.js Outdated Show resolved Hide resolved

nzakas reviewed Jun 27, 2024

View reviewed changes

fasttime force-pushed the fix-external-paths branch from fc765d2 to 77d7e88 Compare August 18, 2024 16:52

fasttime requested a review from nzakas August 19, 2024 08:14

nzakas previously approved these changes Aug 20, 2024

View reviewed changes

mdjermanovic added the accepted label Aug 25, 2024

mdjermanovic changed the title ~~fix: correctly detect if file is outside base path on Windows~~ fix!: correctly detect if file is outside base path on Windows Aug 25, 2024

eslint-github-bot bot added the breaking label Aug 25, 2024

mdjermanovic reviewed Aug 25, 2024

View reviewed changes

fasttime dismissed nzakas’s stale review via b9371a7 September 4, 2024 04:11

fasttime force-pushed the fix-external-paths branch from 484bdb9 to 2d7af8a Compare September 8, 2024 13:42

fasttime and others added 11 commits October 19, 2024 22:14

fix unit tests in compat

ae5b89a

throw an error if a path is not absolute

6140189

allow relative paths

c8d62a3

don't resolve relative paths in tests

ba5056e

default basePath to "/"

6eaced9

revert changes in compat tests

6fd825b

use private fields

98efade

resolve paths to @jsr/std__path dependency exports

3b90297

use non-capturing group

882c3ec

Co-authored-by: Milos Djermanovic <[email protected]>

fix typos

606adcf

Co-authored-by: Milos Djermanovic <[email protected]>

annotate types of private fields

ab61572

fasttime force-pushed the fix-external-paths branch from 98b9a76 to ab61572 Compare October 19, 2024 20:14

add dot

d72dd7d

nzakas previously approved these changes Oct 21, 2024

View reviewed changes

disallow empty strings for basePath

4ed3c1c

fasttime dismissed nzakas’s stale review via 4ed3c1c October 23, 2024 06:01

update docs

8c30779

nzakas reviewed Oct 23, 2024

View reviewed changes

fasttime added 2 commits October 24, 2024 10:09

improve basePath validation

fb712b1

remove unnecessary async keword from test functions

949b364

mdjermanovic reviewed Oct 24, 2024

View reviewed changes

nzakas approved these changes Oct 24, 2024

View reviewed changes

mdjermanovic approved these changes Oct 24, 2024

View reviewed changes

mdjermanovic merged commit f93aa4c into main Oct 24, 2024
14 checks passed

mdjermanovic deleted the fix-external-paths branch October 24, 2024 19:01

github-actions bot mentioned this pull request Oct 24, 2024

chore: release main #125

Merged

fasttime added a commit to eslint/eslint that referenced this pull request Oct 28, 2024

fix: update for eslint/rewrite#59

3cd2d77

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix!: correctly detect if file is outside base path on Windows #59

fix!: correctly detect if file is outside base path on Windows #59

fasttime commented Jun 17, 2024 •

edited

Loading

fasttime Jun 17, 2024 •

edited

Loading

fasttime Jun 17, 2024

fasttime Jun 17, 2024

nzakas Jun 27, 2024

fasttime Jun 28, 2024

nzakas Jul 1, 2024

fasttime Jul 8, 2024

nzakas commented Aug 8, 2024

fasttime commented Aug 13, 2024

nzakas commented Aug 13, 2024

fasttime commented Aug 19, 2024

nzakas commented Aug 20, 2024

nzakas left a comment

mdjermanovic Aug 25, 2024

mdjermanovic Aug 25, 2024

fasttime Aug 25, 2024

nzakas Aug 26, 2024

fasttime Aug 27, 2024

nzakas commented Sep 3, 2024

fasttime commented Sep 4, 2024

mdjermanovic commented Sep 4, 2024

fasttime commented Sep 8, 2024

nzakas left a comment

mdjermanovic commented Oct 22, 2024

fasttime commented Oct 23, 2024

nzakas Oct 23, 2024

fasttime Oct 24, 2024

mdjermanovic Oct 24, 2024

nzakas left a comment

mdjermanovic left a comment

		@@ -1 +1,2 @@
		package-lock = false
		@jsr:registry=https://npm.jsr.io

		const filename = path.resolve(basePath, "foo.js");
		const filename = "foo.js";

fix!: correctly detect if file is outside base path on Windows #59

fix!: correctly detect if file is outside base path on Windows #59

Conversation

fasttime commented Jun 17, 2024 • edited Loading

Prerequisites checklist

What is the purpose of this pull request?

What changes did you make? (Give an overview)

Related Issues

Is there anything you'd like reviewers to focus on?

fasttime Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nzakas commented Aug 8, 2024

fasttime commented Aug 13, 2024

nzakas commented Aug 13, 2024

fasttime commented Aug 19, 2024

nzakas commented Aug 20, 2024

nzakas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nzakas commented Sep 3, 2024

fasttime commented Sep 4, 2024

mdjermanovic commented Sep 4, 2024

fasttime commented Sep 8, 2024

nzakas left a comment

Choose a reason for hiding this comment

mdjermanovic commented Oct 22, 2024

fasttime commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nzakas left a comment

Choose a reason for hiding this comment

mdjermanovic left a comment

Choose a reason for hiding this comment

fasttime commented Jun 17, 2024 •

edited

Loading

fasttime Jun 17, 2024 •

edited

Loading