pathGroups using minimatch instead of real regexes

[ksjogo]

The new pathGroup addition is using minimatch, this being based on shell glob expansions makes it incredible hard to do differing matching on paths containing ../ updots, as these are not easily (if at all) added to the minimatch expression in unspecified depth.
Yes, the codebase should import from the roots and not jump up arbitrarily, but sometimes the world is at it is.
This shortcoming is not evident inside the PR tests themselves, as all these prefix with special characters: https://github.com/benmosher/eslint-plugin-import/pull/1386/files
I didn't find a way to express my wanted regex with minimatch, it would be great to have a shortcutting flag to just supply a real regex.

[ljharb]

I find regexes in config files to be a massive antipattern, and am not interested in adding that.

Can you give me a concrete example of a pathGroups setting you're trying, and the code you want it to work on?

[ksjogo]

Yes, the codebase should indeed use proper root based imports and no relative ones, but that isn't necessarily the case.
Our current configuration:

"import/order": [
			"error",
			{
				"newlines-between": "always",
				"alphabetize": {
					"order": "asc",
					"caseInsensitive": true
				},
				"groups": [
					"builtin",
					"external",
					"internal",
					"parent",
					"sibling",
					"index"
				],
				"pathGroups": [
					{
						"regex": "(..\/)*(actions|graphql|hocs|sagas)\/?.*",
						"group": "internal"
					},
					{
						"regex": "[.]\/components|[.]\/containers|components\/SVG",
						"group": "internal",
						"position": "after"
					},
					{
						"regex": "([.]{2}\/[A-Z][a-zA-Z]+)|(^[.]{2}\/SVG)",
						"group": "parent",
						"position": "before"
					},
					{
						"regex": "[.]{2}\/[a-z][a-zA-Z.\/]+$",
						"group": "parent",
						"position": "after"
					}
				]
			}
		],

Which runs perfectly fine with my fork with proper regexes.
These seem to be unable to be approximated by minimatch because minimatch is doing hard splits on every / as globs are built to match to the specific directory structure given, whereas in this case the amounts of ../ will actually vary between files and require different grouping.
I have a fork on my work computer, which I will upload tomorrow. Basically just adding a regex to the config options, making the schema oneOf either and then just constructing a regex object and matching that.

[ljharb]

I'm not interested in a regex approach; I'd rather fix the way we're using minimatch.

In other words, I'd expect you to be able to use globs - multiple globs in an array if possible - to achieve what you want, and if you can't right now, let's make that happen.

[ksjogo]

What's the reason for not wanting to support regex?
They are better known then globs usually and minimatch is creating these anyway (actually more complex ones then I posted here).

[ljharb]

Supporting regexes in eslint configs leads to all sorts of CVE vulnerabilities about catastrophic backtracking, for one - but also I think it's overpowered, and it'd be better to explicitly support the needed features.

As for "better known", globs are what eslint itself uses, as well as gitignore and npmignore, so I'm not sure that's true.

[ksjogo]

Okay, didn't think about redos here. Was there some concrete example for this in the past, I am unfamiliar with eslints internal happenings and in which case an attacker would be able to inject things into my eslint config (apart from all the code usually pulled unseen from npm anyway, which could do whatever anyway?).

I pretty much remember my university modules about regex and FSMs, never had any deeper looks into globs. :D

I suppose we could add some way to strip of suffixes from the name via config before minimatching and achieve the same.

[ljharb]

Yes, there was a plethora of CVEs filed on eslint plugins a year or three ago for exactly this.

[tulir]

I also want to match an arbitrary number of parent directories.

I have a web app that imports external dependencies from /web_modules/ (using snowpack). However, I want it to be able to run on arbitrary paths rather than requiring dependencies to be at the root of the domain or on a specific subpath, so even the external dependency imports are relative.

I'm currently using {.,..,../..,../../..}/web_modules/** as the pattern. It's very ugly and doesn't support arbitrary amounts of nesting, but works fine for now.

[ljharb]

@tulir **/web_modules/** doesn't work for that?

[tulir]

Pretty sure it doesn't:

> minimatch("../../web_modules/preact.js", "**/web_modules/**")
false
> minimatch("../../web_modules/preact.js", "{.,..,../..,../../..}/web_modules/**")
true

[ljharb]

I'm not too convinced by snowpack as a use case, since this plugin relies on node_modules living on disk, and everything coming from npm install.

However, this seems like it would be better solved by using a snowpack resolver instead of the node resolver + a bunch of rule configuration overrides.

[ksjogo]

Would you be okay with adding an option that strips of an arbitrary amount of ../ before matching?

[ljharb]

That still seems like a hacky workaround for not using a snowpack resolver, if you're using snowpack.

[Menci]

minimatch is buggy -- isaacs/minimatch#30 which stops path like ./aaa, ../aaa be matched together with @internal/aaa by **.

[ljharb]

That doesn't seem like a bug to me. ** doesn't grab dotfiles.

[Menci]

So how on the earth can I match both @/aaa/bbb/ccc/foobar and ./foobar with minimatch?

[ljharb]

@Menci ['**/foobar', '**/.*/foobar']?

[Menci]

@ljharb Thanks. I didn't see in the docs that the pattern can be array.

[Nantris]

Issue closable now? I enthusiastically agree that globs are the optimal approach here.

[ksjogo]

The issue remains.
And I enthusiastically think that not much if anything is gained by not having regexs.
But if the powers that be want to restrict the power, it probably wont fix.

[ljharb]

A ton is gained - regexes in eslint config are the sole source of CVEs for the eslint ecosystem afaik, and by avoiding them, we avoid an entire class of vulnerability.

[ksjogo]

I am still not clear in which case someone being able to add a regex inside my eslint config is the worst thing that party would be able to do, i.e. in which case would a party be able to add a regex to my config but not to be able to modify my codebase in some other way?

[ljharb]

I agree it's not a legitimate attack vector. However, that doesn't stop the security industry from generating tons of CVEs and thus github and npm audit warnings every time there's a regex in an eslint rule anywhere. The cost isn't worth the benefit.

[ksjogo]

Ah, okay, I wasn't aware of that angle. That indeed would be quite annoying.

Maybe I should generate some exploit report related to ../../../ paths and force prefixes on everyone. :D

[ljharb]

off topic: ../ paths are good, because when they get ugly it means it's time to move things to a separate package.

[ksjogo]

Coincidentally Dan wrote something about regex vulns:
https://overreacted.io/npm-audit-broken-by-design/

Then it won’t include outdated flexbox hacks in the output. Since multiple tools rely on the same configuration format for the browsers you target, Create React App uses the shared browserslist package to parse the configuration file.

So what’s the vulnerability here? “Regular Expression Denial of Service” means that there is a regex in browserslist that, with malicious input, could become very slow. So an attacker can craft a special configuration string that, when passed to browserslist, could slow it down exponentially. This sounds bad…

Wait, what?! Let’s remember how your app works. You have a configuration file on your machine. You build your project. You get static HTML+CSS+JS in a folder. You put it on static hosting. There is simply no way for your application user to affect your package.json configuration. This doesn’t make any sense. If the attacker already has access to your machine and can change your configuration files, you have a much bigger problem than slow regular expressions!

Okay, so I guess this “Moderate” “vulnerability” was neither moderate nor a vulnerability in the context of a project. Let’s keep going.

Verdict: this “vulnerability” is absurd in this context.

Let's hope the tooling will change.

[movahhedi]

For those trying to select multi-level parent directories,

const longParentPath =
	"{.," +
	Array.from({ length: 10 }, (_, i) => "../".repeat(i + 1).slice(0, -1)).join(",") +
	"}";

So longParentPath will be {.,..,../..,../../..,../../../..,../../../../..,../../../../../..,../../../../../../..,../../../../../../../..,../../../../../../../../..,../../../../../../../../../..} (10 levels deep).

Use it in a .eslintrc.cjs file like:

{
    "import/order": [
        "warn",
        {
            groups: [
                "builtin",
                "external",
                "internal",
                "parent",
                "index",
                "sibling",
                "unknown",
            ],
            pathGroups: [
                {
                    pattern: `${longParentPath}/Tables/**`,
                    group: "parent",
                    position: "before",
                },
                {
                    pattern: `${longParentPath}/Models/**`,
                    group: "parent",
                    position: "before",
                },
                {
                    pattern: `${longParentPath}/Components/**`,
                    group: "parent",
                    position: "before",
                },
                {
                    pattern: `${longParentPath}/images/**`,
                    patternOptions: { partial: true },
                    group: "unknown",
                    position: "before",
                },
            ],
            "newlines-between": "always",
            distinctGroup: false,
        },
    ],
}