Autodiscovery of *.js configs

[kachkaev]

I’m working on creating @company/markdownlint-config and have a question about this statement in the docs:

JS configuration files must be provided via the --config argument; they are not automatically loaded because running untrusted code is a security concern.

Given this limitation, it’s unclear how to keep the CLI config in sync with the editor integration (e.g. the VSCode one). Tools like Prettier and ESlint autoload *.js configs and it not a security concern for them. Because CLI does this, so can the editor.

Meanwhile, creating a custom package with the config and referring to it in a js files is problematic. While it is possible to feed this ‘unsafe’ file to CLI via --config, the editor won’t see this and will report wrong problems as you type. Ideally, I’d like to allow users of a shared config to just do this:

// .markdownlint.js
module.exports = require("@company/markdownlint-config")

Could .js files be made autodiscoverable too? I understand that theoretically this is a security hole of some sort, but the experience of other tools suggests that it’s fine in practice.

cc @alejandroclaro (author of #85) and @DavidAnson (author of a1f9a15)

[kachkaev]

Alternatively, we could probably allow this in .markdownlint.json, like in Prettier:

"@company/markdownlint-config"

Ideally, this should resolve to discovered package dir → main file in package.json. To avoid using ‘unsafe’ require() (which would resolve path to package and look into its package.json, but succeed for js files too), the resolution could be done manually to make sure it only works for json and yml files.

[DavidAnson]

I don’t want to knowingly expose a security vulnerability (even if other packages do), but I’m not sure we need to. If I understand correctly, you are OK with the way this feature works in the CLI today, but are concerned with how it would work in the VS Code extension. I think this is a similar situation as custom rules which are already implemented in both places. Using a custom rule with the CLI requires passing a command line argument; doing so in the extension pops a dialog that asks the user to confirm they are going to run external code. They can allow it once or allow it permanently for that project. I think a similar approach should work here – if the JS config file is detected by the extension, prompt the user to confirm it should be used. If so, then it can be loaded and executed similarly to how the CLI handles this. Does that seem like it would work?

[kachkaev]

👋 @DavidAnson, thanks for your reply! I understand your motivation to not add *.js autodiscovery just because others do and, to be honest, if we dig deeper into the problem I'm trying to solve, taking the *.js route is not strictly necessary.

The only reason why I was wondering about *.js is this function call: require("@company/markdownlint-config"). By using require, we invoke Node’s require.resolve() and thus turn @company/markdownlint-config into ./node_modules/@company/markdownlint-config/whatever.json under the hood.

At the moment, if we store config in the autodiscoverable .markdownlint.json, we can’t just write:

{
  "extends": "@company/markdownlint-config",
  "some-extra-rule": true
}

It has to be:

{
  "extends": "./node_modules/@company/markdownlint-config/whatever.json",
  "some-extra-rule": true
}

Such a hardcoded file exposes unnecessary details and is thus less friendly and more fragile.

Config autodiscovery is an important feature IMHO, because it lets CLI and the editor stick with the same set of rules without any action from the user. Because of that, --config=.markdownlint.js seems worse than writing an ugly path.

WDYT of applying require.resolve to config["extends"] in JSON? This will make config sharing easier, while still keeping the workflow safe. The change looks fully backwards-compatible, I can submit a PR if it helps.

[DavidAnson]

I like the idea, but have a question. In the @company/markdownlint-config case you are trying to enable, doesn’t that reference always point to an npm package directory? I’m not sure that’s enough to go on; there could be many files in that directory which could be used to define the configuration. You might suggest that we standardize on looking for .markdownlint.json there, but that may just as easily be used by the package for its own purposes. I might be thinking about this wrong, but it seems to me that referencing a package will require running the code of that package’s entry point in order to resolve its exports?

[kachkaev]

When you call require.resolve("some-package-name"), Node.js opens package.json in ./node_modules/some-package-name and searches for the value in the main field (e.g. "main": "whatever.json"). Then it concatenates two path segments and returns ./node_modules/some-package-name/whatever.json. No code in the userland gets executed 🙂

👀 docs on require.resolve()

[DavidAnson]

That’s great, but it feels like there will need to be two different types of packages: one that executes code in order to support the existing .markdownlint.js feature in the same manner as ESLint, and another that references a JSON file in the manner you just outlined. I’d worry that people will confuse the two kinds of packages and also that they won’t be able to use the same package for both scenarios. Does that make sense?

[kachkaev]

Conceptually yes, there could be two types of packages: those saying "main": "whatever.json" and "main": "whatever.js". That does not change the game though, because it’s already possible to say "extends": "./node_modules/@company/markdownlint-config/whatever.js" in a local JSON file. Using require.resolve() instead of path.resolve() does not add any complexity, it just makes path resolution a bit fancier (in a backwards-compatible fashion, AFAIU).

---

Giving feedback on importing a js in a safe json-only mode looks like a different ticket. It could be useful even in the current version, i.e. the one that does not use require.resolve().

I was playing with markdownlint yesterday in a monorepo. My first version of .marjdownlint.json contained

{
  "extends": "./packages/markdownlint-config/index.js"
}

This was failing silently without any message about the impossibility of extending js from json.
When I replaced extends js with extends json, all began to work.

---

So I see two independent themes here:

• detecting extends js in a json-only mode and notifying users about the safety switch
• making path resolution a bit fancier by replacing path.resolve with require.resolve, while not affecting the safety system.

Here is the essence of the second theme.

Current instructions for the shared config: README.md
Goal: Changes in the package with the config + Changes in downstream apps

[DavidAnson]

extends never claims to support JS files, I think. It could be explicit about that in the docs. I don’t think it hides all failures when referencing a file, but I can check.

I like your idea of switching to require.resolve if doing so doesn’t change the current security model. I’ll look into that as well.

[DavidAnson]

@kachkaev, in thinking about this in the context of markdownlint-cli2, I'm inclined to allow auto-discovery of .markdownlint.js (like with .markdownlint.json) as you originally requested. Assuming I do so, the rest of the discussion above seems like it becomes unnecessary (sorry, my fault). My updated reasoning in the context of CLI2 is that .markdownlint-cli2.jsonc already allows using (via require) custom rules, markdown-it plugins, and output formatters - and any one of those could be malicious packages or code, and therefore require-ing .markdownlint.js does not introduce any additional attack surface.

Doing the same thing for markdownlint-cli does introduce an additional attack surface as discussed above, but you point out this pattern is widely used by other tools, so that may not be the problem I originally worried about.

[kachkaev]

I see your point @DavidAnson. WDYT of replacing path.resolve with require.resolve in markdownlint-cli though? This does not affect security, yet will improve this use case:
https://github.com/kachkaev/njt/blob/24dc55ef55d1cd2b994a5e7f3eae0af5ed8bb565/.markdownlint.json#L2

[DavidAnson]

The way I read the documentation, replacing one call with the other would break some path scenarios that work today. I'm thinking that it may be necessary to use both approaches in order to preserve support for paths and also add the support for modules as you are asking.

[JoshuaKGoldberg]

So, just to be clear, is this issue resolved by #342?

[DavidAnson]

I don't think this issue is resolved: the projectConfigFiles array still only contains JSON/YAML files.