-
-
Notifications
You must be signed in to change notification settings - Fork 270
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
Add setting: exclude directories from linting (globally/default and per linter) #1711
Comments
Not sure if Sublime plugins generally do prompts, but adding a "A version of |
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 So what we currently have is an opt-out situation. Automatic by default, set 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) ... |
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. |
Windows Defender will detect malicious software on |
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 |
Great feedback!
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
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.
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
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. |
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. |
Well there is a reason some people think the very presence of nodejs on a system means it’s compromised 😉 |
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. |
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. |
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. For files you just drop into a window ("loose" files). Of course the simple excludes without the "!": 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 ( There is a confusing (for end-users) implication here because we have "double inheritance" then. We have the "global" settings in
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. |
Only problem with the current Adding a global |
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. |
About the security here: project settings aren't loaded automatically, e.g. if you just use Whitelisting would be an option here. (Implemented as a pre-condition in 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: |
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. |
And do you think there is a practical (for end-users) difference between using |
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 |
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. |
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:
So what is a "pattern"? I think it could be like the existing |
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 runnpm 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 ^_^.
The text was updated successfully, but these errors were encountered: