reconsider whether a project's .gometalinter.json can override default linter commands

[marienz]

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 https://github.com/golang/go/issues/22125) and build (see https://github.com/golang/go/issues/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.

[dnephin]

merely looking at freshly checked-out code will then silently run commands

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.

don't support the "Linters" configuration key unless the configuration file was loaded from the user's home directory

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.

it means running gometalinter on a freshly checked-out project can run arbitrary commands specified in that project's .gometalinter.json

Yes it does. But you can also execute arbitrary commands with go test and go generate. I see running lint as a very similar thing, and it's much easier to check what linters will be run than reading every line of test code.

Personally I think it's good the way it is now.

[alecthomas]

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.

SublimeLinter-contrib-gometalinter does this.

Yes it does. But you can also execute arbitrary commands with go test and go generate. I see running lint as a very similar thing, and it's much easier to check what linters will be run than reading every line of test code.

Yes, good point. Agreed.

[marienz]

Do editors typically run linters when you open a file?

syntastic's recommended config, ale and flymake all check on open (I didn't check other editors).

I thought they ran when you saved a file. You shouldn't need to save a file to view it.

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.

But you can also execute arbitrary commands with go test and go generate.

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.

I see running lint as a very similar thing

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.

and it's much easier to check what linters will be run than reading every line of test code.

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).

don't support the "Linters" configuration key unless the configuration file was loaded from the user's home directory

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.

What I'm suggesting is that gometalinter support two different kinds of configuration file:

• In ~/.gometalinter.json (or $XDG_CONFIG_HOME/.gometalinter.json, or a file specified on the commandline, etc), support all possible changes. This includes (re)defining linters. This is safe: if those files got there without me knowing about them, I have bigger problems.
• In the .gometalinter.json bundled with the project being linted, only support toggling linters on/off or adjusting their parameters.

(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:

• Vim and Emacs both support "modelines", where the text file being edited changes some of the editor's settings. These settings can also be modified in the editor's configuration file. But when changed through a modeline, the settings that can be changed and/or the values they can be changed to are limited. (Several editors also support a thing called EditorConfig which fulfills a similar purpose, but that's not as good an example: that config file has to be simple for cross-editor support, not just security).
• Several DVCSes (such as git and mercurial) support configuration inside repositories as well as in the user's home directory. The per-repository configuration is either not cloned along with the repository (has to be explicitly created) or limited.

[dnephin]

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.

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

Really? Storing config in the repo is actually the norm, not the exception: tox.init, setup.py, package.json, Gruntfile, gulpfile.js, Dockerfile, docker-compose.yml, Rakefile, all CI config (circleci, travis, Jenkinsfile, appveyor, codecov, etc).

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 gometalinter is a problem for most cases: a user running gometalinter locally or as part of CI. The only problem is that an editor can be configured to run gometalinter on a project, so let's fix that problem.

Without any changes to gometalinter you could configure the editor to use --config ~/.gometalinter.json. That way it will ignore whatever config is included with the project. We could also add a --no-config flag (or support --config=none) to disable loading the default config, and suggest that editors use that flag as the default.

[marienz]

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.

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.

Really? Storing config in the repo is actually the norm, not the exception: tox.init, setup.py, package.json, Gruntfile, gulpfile.js, Dockerfile, docker-compose.yml, Rakefile, all CI config (circleci, travis, Jenkinsfile, appveyor, codecov, etc).

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).

I don't think the current behaviour of gometalinter is a problem for most cases: a user running gometalinter locally or as part of CI.

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).

Without any changes to gometalinter you could configure the editor to use --config ~/.gometalinter.json. That way it will ignore whatever config is included with the project

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.

[dnephin]

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

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.