Skip to content

Support external ESLint configs #86

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

Closed
caiges opened this issue Mar 28, 2016 · 20 comments
Closed

Support external ESLint configs #86

caiges opened this issue Mar 28, 2016 · 20 comments

Comments

@caiges
Copy link

caiges commented Mar 28, 2016

For my organization's projects and my own I use an external npm package that houses my eslint config. It would be nice to support this workflow as eslint configs change slowly so sharing via npm package or similar means is a fairly common practice. This workflow is described in ESLint's shareable configs documentation.

@cilindrox
Copy link

Probably related to #75

@dblandin
Copy link
Contributor

Hi @caiges,

We don't currently support ESLint plugins or shared configs via NPM due to security concerns. We've made a few exceptions for popular plugins that we now include within the engine. What we recommend for now is to keep a shared config within the repo and periodically update it.

Here's a good example:

https://github.com/sagiegurari/simple-oracledb/blob/master/.codeclimate.yml
https://github.com/sagiegurari/simple-oracledb/blob/master/.eslintrc.js
https://github.com/sagiegurari/simple-oracledb/blob/master/project/config/eslintrc-common.json

Here at Code Climate we have a separate repo which holds our shared configs.

Sorry I don't have a better approach to suggest but that's what we can support today.

Thanks!

@caiges
Copy link
Author

caiges commented May 31, 2016

Thanks for the info @dblandin. Could you elaborate on the security issue if you can? I'd like to help address the issue if it's something ESLint is doing specifically.

@dblandin
Copy link
Contributor

dblandin commented May 31, 2016

@caiges,

Unlike many popular CI systems, our build infrastructure isn't yet designed to execute customer code. It was designed to run engines statically against source code. Before we allow arbitrary execution of customer code (including NPM packages or RubyGems), we have some work to do to ensure that our systems and customer data remain secure. Unfortunately, this isn't an issue specific to ESLint. It's a shared concern across our platform.

Another workaround is to use our CLI, which you can run locally and in your own CI environment. You may be able to run npm install and codeclimate analyze to get things to work. Of course you'd miss out on the value codeclimate.com provides.

@caiges
Copy link
Author

caiges commented May 31, 2016

@dblandin Wow, great response - thank you. This isn't a showstopper for us and it's nice to know you're thinking about the issue.

@dblandin
Copy link
Contributor

@caiges My pleasure! Thanks for the support!

@mansona
Copy link

mansona commented Jul 29, 2016

Right, so this one is a showstopper for us 😭 Has there been any movement to support third-party shared configs yet? Essentially I want what codeclimate.com is providing i.e. showing improvements on a branch/PR level

I have noticed from #75 that you support the airbnb style so a workaround for us would be to be able to configure a custom "extends" in the .codeclimate.yml file because our custom shared config is quite close to theirs. This would allow us to use the .eslintrc.json file locally with our local development and "put up with" a few false positives on code climate.

I hope all this makes sense 😂

@maxkoryukov
Copy link

The same case: we have our own ESLint config in a separate NPM package, it is inherited from one of the common-well-known configs, but we want to use other syntax guidelines..

And now we are unable to use Codeclimate , because custom configs (enclosed in packages) are not supported...

@pbrisbin
Copy link
Contributor

Hey folks. I just wanted to let you know that I created a sticky issue at qltysh/qlty#480 to document this limitation, some of the reasoning behind it, and provide an avenue for any updates from our side. I'm going to go ahead and close this issue here, because the limitation is broader than any one engine.

@mansona the extends idea sounds promising, but I'm not sure I completely follow. Would you be able to open a new feature request that outlines it in more detail? A PR that attempts to implement it would be ideal.

@mansona
Copy link

mansona commented Aug 25, 2016

I'll split my reply into two comments:

To explain my "extends" idea: Essentially I have 3 tiers of rules being applied to my eslint now, the vast majority coming from Airbnb's external config. Next, I have a .eslintrc.json file that makes specific changes that I need so for my current projects:

{
    "env": {
      "node": true
    },
    "extends": "airbnb",
    "rules" : {
      "new-cap": [2 , { "capIsNewExceptions": ["Q", "ObjectId"] }],
      "no-underscore-dangle": [2, { "allow": ["_id"] }]
    }
}

The idea of having my own external config was so that I didn't have to maintain this ^^ exception based eslint.json on every repo and change all our repos once we need to change something.

The last tier of the solution is to add this to our .codeclimate.yml file:

    enabled: true
    channel: eslint-2
    checks:
      import/no-unresolved:
        enabled: false 

This means that the import/no-unresolved rule works locally for us but doesn't create issues on code climate.

This issue has already had a chilling effect on our development. We want to continue with our own slightly custom coding style, but we have had to remove a lot of the customisations we were using because having Code Climate is more valuable to us than having our style followed. Just to be clear this is not a good thing.

If you are not going to allow us to use the standard method of including custom configs, I would recommend that you do something on the backend like allowing us to define our own rules in our accounts that can apply to all our applications. This way we could have our local @stonecircle ruleset, and you could have a server-side definition of rules that contains and all would be right with the world 🎉

@mansona
Copy link

mansona commented Aug 25, 2016

@pbrisbin I am commenting here instead of qltysh/qlty#480 because that issue is locked, are you planning to open that up for further discussion?

I know that you have a major security issue to contend with here (and I'm not a security professional) but I don't understand why this isn't a solved problem already? Travis-ci already allows you to install any npm package your want, surely we can re-use some of their solutions for this?

@pbrisbin
Copy link
Contributor

pbrisbin commented Aug 29, 2016

About your extends idea:

I would recommend that you do something on the backend like allowing us to define our own rules in our accounts that can apply to all our applications

Org-wide config is an initiative we hope to take on soon, potentially next quarter. It's a meaty project but something that would provide great value. This feature is broader than any one engine.

It doesn't sound like your suggesting any changes that would make sense within this engine specifically to support your use-case better, is that correct?

About the locked issue:

that issue is locked, are you planning to open that up for further discussion?

Not at this time. We believe we understand the limitations (we feel the same pain as our users), and that Issue is meant as a notification and way we can provide updates going forward, not a mechanism for wider discussion, or +1 comments. I apologize if you feel you're being silenced; thanks for raising that concern here.

isn't [this] a solved problem already? Travis-ci already allows you to install any npm package your want, surely we can re-use some of their solutions for this?

We may be being overly paranoid here, but we'd still prefer that. The difference is that Travis will let you run commands you wrote via your configuration file. With our system, there is code a 3rd party wrote (open source engines) being run with access to your source code. It's not that we specifically want to prevent npm package installs, we want to prevent a malicious engine from taring your project onto an attacker's server. We prevent this today by restricting networking; preventing npm package installs is an unfortunate side-effect.

To be clear: we don't suspect anything nefarious to be going on in the community-provided engines, and we do vet them in many ways (for quality and security concerns). But we feel it's important to maintain a last-line-of-defense at the network layer to ensure that any future mistakes on our part don't result in our customers losing valuable IP.

@aaroncraig10e
Copy link

I realize this is an old thread and perhaps this stuff is no longer really on your radar, but this single issue causes a major headache for our organization (and I imagine many others).

The assertion that by refusing to run eslint plugins that you haven't vetted somehow protects an ignorant developer from having their source code stolen is dubious at best as it assumes that the developer in question isn't running eslint checks on their own local machine, where the evil plugin could do all the damage it wanted well before hitting your service.

If this is the only reason we can't use popular plugins (ie eslint-import-resolver-webpack) then I hope you will relent, as I currently have to go through and manually mark 100s of resolver errors as invalid.

@pbrisbin
Copy link
Contributor

Hi @aaroncraig10e, sorry this is causing you headaches.

The assertion that by refusing to run eslint plugins that you haven't vetted somehow protects an ignorant developer from having their source code stolen is dubious at best as it assumes that the developer in question isn't running eslint checks on their own local machine, where the evil plugin could do all the damage it wanted well before hitting your service.

No one's made this assertion and what you describe is not at all our motivation.

It's not that we specifically want to prevent npm package installs, we want to prevent a malicious engine from taring your project onto an attacker's server. We prevent this today by restricting networking; preventing npm package installs is an unfortunate side-effect.

Our stance is that there is security risk with allowing engines to make network calls. Running a shared plugin that's not pre-installed requires a network call to install it. Therefore, we cannot run plugins that are not pre-installed in the engine image.

We do not "vet" the plugins. We just limit what we pre-install to those with reasonable adoption. This is just a trade off between the value of having the plugin there (dependent on how many users would use it) and impact of having a larger image to operate (which can impact run and deploy time for everyone).

Now, the import-resolver-class plugins actually have their own, completely separate issue. Unfortunately, these plugins are incompatible with the way Code Climate Engines are run because they make assumptions about the file system which are not true when run as an Engine. We at one point added just that plugin, but had to remove it later.

I currently have to go through and manually mark 100s of resolver errors as invalid.

Have you considered ignoring those check(s) via .codeclimate.yml?

I hope this reply helped clarify some things.

@aaroncraig10e
Copy link

Thanks for the quick response.

Unless I'm misunderstanding, I think we are saying the same thing -- your reasoning holds that by restricting networking, a potentially malicious engine could steal source code. I am simply pointing out that any code installed through npm's package.json would probably have already been run on the developer's local machine, where networking is not restricted, and therefore could also steal code directly from the developer's machine. Perhaps I'm not understanding why executing code in your environment is somehow different from executing the same code in another environment?

Regarding the suggestion that I ignore the checks via .codeclimate.yml, I tried that. Configuring to turn off import checks still provokes the error, probably because the eslint engine still tries to parse this bit in .eslintrc.json:

  "settings": {
    "import/resolver": {
      "webpack": {
        "config": "./web/webpack.config.dev.js"
      }
    }
  },

I suppose I could make another eslint config file just for Code Climate, but that would then mean I have to maintain two eslint configs, which is sub-optimal.

@pbrisbin
Copy link
Contributor

your reasoning holds that by restricting networking, a potentially malicious engine could[n't] steal source code. I am simply pointing out that any code installed through npm's package.json would probably have already been run on the developer's local machine, where networking is not restricted, and therefore could also steal code directly from the developer's machine.

Again, we're not targeting that attack vector here. We are protecting against an engine doing something malicious. As a concrete example, if someone compromised our docker registry and replaced codeclimate/codeclimate-eslint:latest with their own image, that image would have access to customer source code. With our current policy of not allowing engines network access, we are protected against it being sent somewhere.

Perhaps I'm not understanding why executing code in your environment is somehow different from executing the same code in another environment?

It's not. When you run a codeclimate engine, its networking is restricted. This is true if run locally via the codeclimate CLI or on our hosted environment.

@aaroncraig10e
Copy link

I see, that makes more sense. I hadn't understood that you were concerned with a breach in your own security.

At the risk of beating a dead horse, I still wonder whether or not that's a useful tactic, seeing as how if someone manages to get enough access to replace the code in one of the engines that you maintain, you probably have a lot more to worry about than just having your client's code stolen.

@e-e-e
Copy link

e-e-e commented Oct 4, 2017

We also encountered this problem with our shared projects. I ended up getting around it by writing a simple utility to recursively reduce inheritances. It might be helpful for others encountering this issue - https://www.npmjs.com/package/eslint-reduce

We added this to the prepublish hook of our shared config, so that whenever we publish a new version we also share the reduced version without the external dependencies.

It basically looks like this:

scripts": {
  "prepublish": "npm run build",
  "build": "eslint-reduce -v -f index.js -o ./dist/.eslintrc.json"
}

Because code climate does not run npm install you then also need to commit the file to your git repository. Our git ignore now has this to allow it to work properly.

# Dependency directory
node_modules/
# do not ignore eslint prebuilt for code climate
node_modules/eslint-config-loanmarket-react/*
!node_modules/eslint-config-loanmarket-react/dist

and our codeclimate.yml also points to our node module

eslint:
    enabled: true
    channel: "eslint-4"
    config:
      config: ./node_modules/eslint-config-loanmarket-react/dist/.eslintrc.json

I hope someone else finds this useful.

@Xosmond
Copy link

Xosmond commented Nov 27, 2017

Did you know a way to use .eslintignore on codeclimate.yml ?

@dblandin
Copy link
Contributor

Did you know a way to use .eslintignore on codeclimate.yml ?

@Xosmond I think this question is better suited for a separate issue but the engine should already support a .eslintignore file. The ESLint tool should pick that up and use it automatically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants