Add setting: exclude directories from linting (globally/default and per linter)

[Jimbly]

As discussed in #1710, it's a major security issue that simply opening a file enables running of arbitrary executable code in the file's directory, making it inherently unsafe to even simply view a file from an untrusted source in Sublime Text if you have SublimeLinter and any relevant linter installed. As an example, if you clone this repo or open this .zip file, and open the .js file in SublimeText with SublimeLinter-eslint installed, and it will run the arbitrary code (dropping a file in your home dir / on your desktop).

To be safe, by default, SublimeLinter should only run executables installed by the user (or provided by SublimeLinter- packages if that's done), not file-relative executables. The existing behavior of searching relative to the linted file should still be enableable at the user level. Arguably not per-project, otherwise, obviously, a malicious project would just change the option in a project file, but opening a .sublime-project file from an untrusted source is already a bit sketchy, so that might be reasonable to allow, and would satisfy most common use scenarios.

I'd also argue this is a usability issue, as more than once I've been bitten by SublimeLinter stopping working in some folders, for, at the time, unknown reasons, but probably due to issues like #1710, but probably also when opening freshly cloned repos that I wouldn't trust enough to run npm install in, or often when traveling, didn't have the bandwidth to run npm install in.

I realize that any packages delivered through Package Control or NPM are significant security risks, as the authors can generally all execute arbitrary code at install- and run-time, however that has a layer of trust and accountability that doesn't exist when you just want to look at the source code for a cool thing someone just posted to Reddit ^_^.

[Jimbly]

Not sure if Sublime plugins generally do prompts, but adding a "A version of eslint was found in the current project, would you like to run that instead of your OS installed version? Yes/Always/No/Never" prompt would let people trivially transition if this change were made.

[kaste]

That's a difficult one. I actually wouldn't want to make significant UX changes without some form of community behind it. Generally, this kind of exploit is possible in eg vim/ALE and VsCode as well. All linter frameworks prefer locally found executables or installations over global ones. Iirc vim/ALE doesn't even use a global eslint without explicit opt-in by the user. Of course, all people want and love the automation here.

An automatic popup is a no-go. Users just hit "Yeah, fuck you" or "No, go away" depending on the default we choose. Of course "A version of eslint was found ..." would be a lie because what we found is just an executable file. The point already is that we don't know if it is eslint. Of course the user, confronted with (just) this popup actually distracting her focus, doesn't know either.

I remember from #1710 that you just set executable to eslint. No pun here, because we love that automation. You actually are too lazy, and really we all are, to type in or copy the full absolute path to an executable, and then you hope just giving the name will work, but of course we have to look it up for you. The user actually doesn't know the exact PATH at this point (we're not in a shell!), and iirc the current working dir is always in, at least on Windows. So that's still exploitable.

So what we currently have is an opt-out situation. Automatic by default, set executable to disable some or all automatics. (Absolute path is the only way here.)

The reverse is opt-in. But not opting into "which executable" but imo for every unknown project don't lint at all until the user wants it. That would be my preferred UX anyway but I'm only the main contributor, there is no main user. The desired UX is: I don't need the squiggles and tips as long as I'm not actively writing. I don't need "linting" when I'm reading libraries (esp. standard shipped-with-the-lang libraries 🙄), even when reviewing (the PR is already green or red) ...

[braver]

I dunno, maybe that makes me the "main user". I use linters to warn me about syntax errors, unused variables, stray console.log statements, etc. and I want that on all files all the time. That said I also don't really like style feedback on files that I know are not compliant because they're not mine or they're old or whatever. But I would definitely want a project fully set up with linters and config files to just run according to that spec without me having to do something special.

So I don't see how we do something here that doesn't make the work flow super convoluted. It's not about getting consent. Everyone will simply opt in to running local linters. "Get out of the way with your prompts, I want to get shit done". Requiring users to flip a switch before running a local linter just makes them flip that switch. I doesn't make them check the code of the linter, as if they could just look at a program and see that it's malicious. It's also not like everything called "eslint" in your PATH is somehow guaranteed to be secure and safe.

This should be about doing an actual security check that actually really makes users more secure, not making users jump through a loop. If we can find a solution that does that I'm all for.
Paranoid users can already set linters to manual or disabled by default.

[kaste]

Windows Defender will detect malicious software on git clone or when we execute it. I really don't think it will not.

[braver]

Indeed, I actually don’t expect any problems with truly malicious programs. There will be a layer of protection, or on Unix/Mac permissions will stop it from doing really damage. I expect this vulnerability to be more about pranks, like rm -rf-ing your home dir.

[Jimbly]

Great feedback!

I remember from #1710 that you just set executable to eslint. No pun here, because we love that automation. You actually are too lazy, and really we all are, to type in or copy the full absolute path to an executable, and then you hope just giving the name will work, but of course we have to look it up for you. The user actually doesn't know the exact PATH at this point (we're not in a shell!), and iirc the current working dir is always in, at least on Windows. So that's still exploitable.

I actually tested that before posting my solution (and security complaint) on #1710, and I was unable to get it to execute an eslint or eslint.cmd in the working directory. Looks like path on Windows does not included the current directory, but CreateProcess() and system() and such all check the working directory first by convention. Since SublimeLinter is actually searching the path, it behaves in the same, safe, way on Windows and Linux.

Setting it to just eslint isn't just lazy, it's necessary for me - like all of my interface configuration, I have my Sublime settings synced between all of my devices, and the path to eslint is different on each of them, so needing a full local path would be annoying (though not totally unreasonable, I'm probably unusual on syncing all settings like that ^_^).

It's also not like everything called "eslint" in your PATH is somehow guaranteed to be secure and safe.

It's not guaranteed to be safe, but it is guaranteed to have been placed there by a (relatively) deliberate action by me. All entries in my path on Linux require sudo access to write to. These are a very different class of files than whatever is in a random .zip file I download.

The reverse is opt-in. But not opting into "which executable" but imo for every unknown project don't lint at all until the user wants it.

That is interesting. Squiggles when reading the Node source code and such do tend to annoy me more often then not, but I do also agree with braver that I generally want all views linted by default (though I have my global eslint config ignoring all style rules for exactly the reason he mentions). Even if reviewing a PR that's "green" a lot of old projects I maintain have not been converted to newer eslint rules (or are still using jshint), so it's nice to get the newer warnings. I could imagine linting being off by default, and just need to "enable linting this project" once per project, though that will, I think, probably annoy more people than the current spurious linting would (and much more than imagined security exploits).

So... for a compromise, how about a "discoverable" option to always use globally installed linters. I'm imagining just another entry in SublimeLinter.sublime-settings looking like:

// Set to true to allow using project-local linters.
// Turn off for increased security when viewing projects from unknown sources
"allow_local_linters": true,

This would default to on, but power-users who might care about this would notice it when browsing settings (I've certainly at least read through the settings file in the past a few times), and could be called out in the docs. When this is off, all of the logic in SublimeLinter that does searching in the local project would be bypassed, and just use the global path instead.

[Jimbly]

Okay, I'm wrong, security by using a global eslint doesn't actually help much at all. In the eslint case, an .eslintrc.js file can simply have a process.exec() call in it or do whatever it wants, and there's (probably) no situation in which we'd want eslint to ignore local config files (would need to pass some very eslint-specific options to it, nothing general in SublimeLinter there, I'd suspect). It's still a nice option for "always just work" reasons though.

It seems that perhaps the only security-conscious thing we could do in SublimeLinter would be an option like @kaste suggested where it simply does not lint in unknown/newly-opened projects or loose files. That being said, I'm not sure how Sublime handles projects vs files, if I double-click on a loose .js file, it seems to choose randomly between my open projects, and often open it in the context of a primary work project (that would certainly have linting enabled), so even that might not help much with security peace of mind.

[braver]

Well there is a reason some people think the very presence of nodejs on a system means it’s compromised 😉

[braver]

unknown/newly-opened projects or loose files

Well, you can probably already do that by setting all your linters to only run manually, and then per project switching the ones you need to automatic. How that exactly turns out when you drag a separate file into a project window I'm not 100% sure... as you say the boundaries of a "project" are a but fuzzy. It can be figured out, but it's not intuitive. So the settings allow you to create a safe or at least safer environment if that's what you need.

I'm not sure there is more we can do. Requiring users to approve "safe" projects or directories doesn't seem like a practical security improvement.

However, I do kinda like the idea of a setting that tells SL to "only run for files in this directory" (e.g. my development workspace directory). Or perhaps even better, a setting to exclude certain directories (e.g. ~/Desktop and ~/Downloads). Not necessarily for security, but that would actually be pretty practical.

[Jimbly]

However, I do kinda like the idea of a setting that tells SL to "only run for files in this directory" (e.g. my development workspace directory). Or perhaps even better, a setting to exclude certain directories (e.g. ~/Desktop and ~/Downloads). Not necessarily for security, but that would actually be pretty practical.

I like that idea too. Defaulting to not linting ~/Downloads on Windows would probably make it slightly more secure, and generally get in people's face less often when they don't expect it. Having a global include/exclude folder regex list similar to what is in a sublime-project file would be great and would take short work to add to that such that I feel (more) secure opening everything with SublimeText.

[kaste]

Hm, it's like everything we discuss here is already in SublimeLinter, and then something is still missing.

We actually have the "exclude" feature. E.g. you could say, only ever lint stuff in my dev folder. "exclude": ["!C:/Dev/*"] (The "!" for: exclude everyting not in ....).

For files you just drop into a window ("loose" files). "exclude": ["!${folder}*"] (As in: exclude everything not from the open folder of that window.)

Of course the simple excludes without the "!": "exclude": ["C:/Downloads", ...].

The downside is probably that this is a "linter setting", so you would have to repeat this for every linter, and again if you install a new linter, learn a new language.

The other setting that does everything, esp. for my use-case: don't lint projects until I actually participate/write, is of course "lint_mode". Here "manual" set globally would of course prevent automatic linting. But there is no UI to enable linting for specific projects which makes it a bit lame. And there is also no simple switch, "enable all possible, applicable linters for this projects", or simply: "enable automatic linting for this project", because again this is a "linter setting", and I have to enable every linter in the project settings separately. And if I install a new linter again.

Maybe all we need is then a "base linter setting". It's the same object as for the real linters, and all linters inherit from these base settings. (Then the "global" lint_mode (SublimeLinter.lint_mode) would have been a mistake, and would move to said base, e.g. SublimeLinter.linters.base.lint_mode. deprecation policies apply of course)

There is a confusing (for end-users) implication here because we have "double inheritance" then. We have the "global" settings in SublimeLinter.sublime-settings and local view (project) settings. (I'm using flat object notation here.)

  SublimeLinter.linters.eslint.lint_mode (view)
  SublimeLinter.linters.eslint.lint_mode (global)
  SublimeLinter.linters.base.lint_mode (view)
  SublimeLinter.linters.base.lint_mode (global)

Hard to order that list logical since "view" is more special than "global", just as "eslint" is to "base". The first and last lookup key is probably right, but the two middle ones can be flipped, t.i. is "(eslint, global)" more specific than "(base, view)".

Just my thoughts here, not really sure about this one still.

[Jimbly]

Only problem with the current excludes, besides, as you mentioned, needing to specify it for each linter, is that it can be overridden by the project, so even if I have "safe" global excludes set, a malicious project just needs to add excludes: [] and it ignore them. Projects can also override executable which makes my previous "fix" not particularly useful for security (though does still solve other, non-malicious issues, and the executable property does not search the current directory, so it's tricky to actually exploit).

Adding a global excludes setting, which applies to all linters, cannot be overriden by project-local configurations (or, can only be added to, not removed from), and defaults to something sensible like ignoring ~/Downloads would be a nice step, I think. If excludes was merged additively instead of by being replaced, your "base" suggestion would also work here.

[braver]

I think the current per linter excludes setting works especially well from the context of a project. You should be able to fully override it there.
I like the idea of a global excludes setting that says “never ever run here”. Easy to understand and simple to set up. Probably also easy to implement.
Sensible defaults are hard here because Windows, Linux and Mac are so different.

[kaste]

About the security here: project settings aren't loaded automatically, e.g. if you just use subl . or drag a file from a folder that also contains project settings. (Although you can teach Sublime to do so. 🤔) But if they load you could set executable: ["node", "somefile.js"] just assuming some node is on the PATH. You can then also use $folder expansion which is the absolute path to the top folder of such a project, iirc $folder/somefile.js. You can of course set working_dir.

Whitelisting would be an option here. (Implemented as a pre-condition in sublime_linter.hit.) Disallow everything until opted-in. Of course a user could allow C:/Dev and then all sub-projects would follow. Of course shipped with a command "Enable linting of this project.". We already discussed that this not "secure" per se because the user does not know what runs afterwards.

The "base setting" idea is totally biased on bang for the buck. We already have the settings layer, to add this is literally iirc a 4 line diff. It is useful for some settings maybe: disable_if_no_dependency, disable, exclude, and lint_mode. Also the python setting. (Basically you could express use python3.7 by default.)

[braver]

Oh, in my mind this is already no longer about security. I’d like to be able to say “don’t ever bother me about stuff that’s in my downloads folder because I don’t care”. It’s just nice that that also creates a “safe area”. Having a global setting means I don’t have to tinker with settings each time I add a linter. And a global setting also strengthens its use for the security aspect, which is nice but why I’d want it.

[kaste]

And do you think there is a practical (for end-users) difference between using SublimeLinter.linters.base.exclude and some other "truly" global mechanism?

[braver]

No, putting it in a "base" setting is fine. Would other linter settings override it though? Because to me that's kind of what the name "base" implies. Fine either way for me. But if we introduce a "base linter", it would be kind of weird if the styles settings that are now global are not in there.

[kaste]

The settings generally override, we never did deep merging.

Typically you could expect that this works for "styles" as well. This would add 2 lines to the diff 😁 but styles are not your typical setting. You can't override them per project, and only for styles we actually do a merge by concat (or chaining).

TLDR;

All linter settings are actually read and used by the backend except "styles" which is technically not a linter setting but a "view setting" (Not how we get the data or configure a linter, but how we present and transform the data for the user again.) For the user it is though elegant to put these into the "linters" sections. So we can support that of course.

[braver]

We can't really move the styles settings in a backwards compatible way (although in ST4 we can modify settings files while keeping order and comments... so maybe in the future we can upgrade existing settings). So let's leave them.

---

Ok, to wrap all this up into something we can actually do, this is the spec we have in mind:

• create a "base" linter setting key for settings that can be overridden at the linter level
• add support for an "exclude" setting for linters that can have one (string) or more (list of strings) "patterns". all files that match will be excluded from linting

So what is a "pattern"? I think it could be like the existing folder_exclude_patterns in ST, but I don't actually know all the details about how the matching works there. .gitignore can also be a good template. Python has path matching functions that we can use. Perhaps we should just start with a PR that attempts to implement this and go from there.