-
Notifications
You must be signed in to change notification settings - Fork 267
reconsider whether a project's .gometalinter.json can override default linter commands #461
Comments
Do editors typically run linters when you open a file? I thought they ran when you saved a file. You shouldn't need to save a file to view it.
That sounds very strange to me. Is there any tool out there that has such a restriction? Also, isn't most code checked out under the users home directory? Are you suggesting that the config could only be in the home directory itself? That is quite strange.
Yes it does. But you can also execute arbitrary commands with Personally I think it's good the way it is now. |
SublimeLinter-contrib-gometalinter does this.
Yes, good point. Agreed. |
syntastic's recommended config, ale and flymake all check on open (I didn't check other editors).
They do this because they show lint warnings along with the code you're looking at (using underlines or similar), not in a separate compilation window. It makes sense to show preexisting lint issues immediately after opening a file, instead of having all lint issues (old and new) appear only after you make a change.
When I run "go test" I expect it to run the code in the tests I'm pointing it at. When I run "go generate" I expect it to run commands from the package I'm running it on. This behavior is not surprising: these commands exist for the sole purpose of running other code, and neither "go test" nor "go generate" should be run implicitly (see the design doc for "go generate": "Generators should run only when explicitly requested"). If I don't trust the code I'm pointing those commands at, I know to make sure some form of sandboxing is in place.
When I run "git clone", or subsequently run "git" commands on a repo I just cloned, I expect it to run git (not any code from the repo I'm cloning). If "git clone" runs code from the cloned repo, that's a security issue (first example I could find, I think there were better ones). When I run "go get -d", I expect that to run Go (and tools such as git), not the code I'm fetching. If "go get -d" runs code from the repo I'm fetching, that's a security issue. When I run "go get", that still only runs Go (and underlying tools such as git and my system's C compiler). It does not run the code I'm getting, it only compiles it. If it runs arbitrary code, that's a security issue. When I run a word processor on a document, I expect that to only run the word processor. If it also runs code embedded in the document (without my explicit permission), that's a security issue (no link for this one, but look for any word processor with macro support and you'll find it asks for permission before executing macros embedded in documents and/or has had security bugs reported against it when it didn't). I see gometalinter as more like these examples than like "go generate" and "go test". When I run gometalinter on a package, I expect that to only run gometalinter (and its vendored linters), not the code I'm linting. The tree I'm linting should be treated as data by gometalinter, not as code to execute. This includes a .gometalinter.json bundled with that tree: it should treat this as (configuration) data, without executing any part of it.
It's trivial if you realize gometalinter has this feature. But especially with .gometalinter.json being a dotfile, it's fairly easy to miss it's even there, and even easier to not realize it can be used this way. I'd be surprised if any other linters have an equivalent feature (admittedly more by accident than by design: they aren't running subprocesses to begin with).
What I'm suggesting is that gometalinter support two different kinds of configuration file:
(Probably the easiest way of implementing this is to strip the "Linters" key from a configuration file loaded implicitly along with the code being linted, instead of having two completely different configuration files.) I don't know many tools that read configuration files bundled with the data I run them on, instead of only reading configuration from my user's home directory (and/or system configuration directory). Other linters don't make a good example: their configuration files are not powerful enough to need protection. But here are some similar cases:
|
I dislike the idea of storing linter config in the home directory. I use different config for most projects, and I suspect I'm not alone. Every project with multiple contributors will use a different combination of linters. I'm also not a fan of adding multiple config files to solve this problem.
Really? Storing config in the repo is actually the norm, not the exception: This is just a short list of the ones I'm familiar with, I'm sure there are many more examples. All of these can include commands which will be executed. I don't think the current behaviour of Without any changes to gometalinter you could configure the editor to use |
Different configs still work as long as they run linters that are part of gometalinter (including default-disabled ones) or ones configured in the user's home directory. For linters not already part of gometalinter, you currently have to provide the user installation instructions. You would have to add "add this snippet to ~/.gometalinter.json" to those instructions. I agree this is less convenient. Similarly, whitelisting build flags for "go get" is inconvenient.
Yeah, that was poorly worded by me (the way I was thinking of "configuration" is too limited). For many of the tools you listed (I'm not familiar with all of them), running arbitrary commands from the repository I am pointing them at is their sole purpose. I can't reasonably expect something like "make" to do something that does not involve running arbitrary commands from the Makefile of the project I'm building. It is still not intuitive to me that the same applies to a linter. Which of the tools you list are not build systems (along the lines of "make") or CI systems? For the CI systems: which of them let you include commands in the config file that are not run in the context of a (sandboxed) CI environment? Travis has a configuration file and a CLI client, but I'm under the impression the commands from the configuration file are only ever run sandboxed by the CI system, not directly by the CLI client (I could easily be wrong, I'm not very familiar with these systems).
It is a problem for a user running gometalinter locally on a project they did not author (it's equally surprising, although probably less commonly hit than the editor integration case).
I would be sad if that change was made, as being able to control which (preconfigured) linters run, and which of the files in my package are linted, is an amazingly useful feature that I enthusiastically adopted (and being able to add custom linters without having to build a custom gometalinter is a pretty useful feature too, although I currently have no need for it). It means I can just run "gometalinter ./..." (or similar), and have it know to exclude some generated code and run the right linters with the right parameters (tweaking --cyclo-over), and also have editor integration do the right thing. This is similar to what other language's lint tools let me do, so I thought I knew what the limits of the config file would be. With your proposed change, it becomes difficult to still use this gometalinter feature along with editor integration. The editor integration would have to be told which projects are trusted, and run gometalinter with a different --config only in projects that are not trusted (or vice versa). I'd prefer if gometalinter supported a safe configuration file (which I believe is the existing one with the custom "Linters" key removed). I was hoping this is not too inconvenient for most users, as most of them either use linters that are already part of gometalinter (even if default-disabled), or a small number of additional linters they can add to their local config once and reuse across projects. |
This sounds reasonable to me. I think this is a better compromise than introducing confusing behaviour for one specific use case that doesn't make sense for others. |
Support for overriding or adding linters through a configuration file, and for looking for the configuration file in the project tree of the files being linted, are both great. But combining the two might be a little too much of a good thing: it means running gometalinter on a freshly checked-out project can run arbitrary commands specified in that project's .gometalinter.json. This is especially surprising for someone with editor integration set up: merely looking at freshly checked-out code will then silently run commands from its .gometalinter.json.
Contrast this to "go get", which goes to some lengths to check out (see golang/go#22125) and build (see golang/go#23672) code without allowing arbitrary code execution. Viewing the built source should be equally safe.
I'd expect configuration files bundled with source to only control which checks are run on which code (including enabling/disabling named linters) and what the parameters for those checks are, while specifying a new linter or overriding the command of an existing linter needs to be done explicitly (through a commandline flag or a configuration file under the user's control). I think that means: don't support the "Linters" configuration key unless the configuration file was loaded from the user's home directory.
The text was updated successfully, but these errors were encountered: