microsoft / vscode-eslint

VSCode extension to integrate eslint into VSCode
MIT License
1.74k stars 335 forks source link

Popups with every folder I open #1012

Closed kasvtv closed 4 years ago

kasvtv commented 4 years ago

With every project folder that I open in VScode (File > Open folder...) I get the following popup:

"The eslint extension will use the eslint library node_modules/eslint installed locally to the workspace folder 'reponame' for validation. Do you allow this?"

This is really annoying and when the popup window is just closed without giving permission, ESlint will not load at all. This is a huge problem, because it was already hard enough to have teammates respect a linter, let alone when an annoying unexpected popup occurs that one is likely to dismiss, which will not make it load at all.

Is there anything regarding project configuration that will prevent this popup from occurring and just make ESlint work by default like it always did?

jsejcksn commented 4 years ago

Yes, but if it is easy to be overlooked I think it is not good either. It will result in lots of user having ESLint being disabled without knowing it.

This is your opinion (and I'm not saying it's necessarily wrong—it depends on how "lots" is defined), but it's hostile to everyone to continue to use a blocking dialog just because a subset of people don't pay attention to their notifications. That's their own problem, not everyone else's.

bluepnume commented 4 years ago

Yep, for now can we please get a configuration setting to always allow this?

liby commented 4 years ago

What I want to know is that if I choose to Allow: when I open a workspace, the ESlint version of the project may be different from the global installation. Which one has a higher priority? @dbaeumer

chaddjohnson commented 4 years ago

It would be really nice if this didn't pop up every single time I open a file. It's super annoying. A setting or button to globally allow is needed.

dbaeumer commented 4 years ago

@liby the one in the workspace folder wins over a globally installed one.

@bluepnume and @chaddjohnson I am working with the VS Code team on something that gives you more global control about this. If something like always allow is enabled it must be properly reflected in the user interface and this is best done in VS Code and not by the extension itself.

@chaddjohnson this should only happen once for a workspace folder you open (unless you press Escape). If you see that happening differently can you please provide me with steps on how to reproduce.

horidon commented 4 years ago

Hi all, accidentally chose disallow option for my directory in this pop up. Is there any way to show this pop up again or remove a directory from ignore list? Where does the ignore list with directories can be located?

dbaeumer commented 4 years ago

@Balashov-nikita yes, run the command ESLint: Reset Library Decisions

horidon commented 4 years ago

@dbaeumer Thank you!

dinofx commented 4 years ago

I opened a workspace and did something like COMMAND+P to open a file. In the middle of typing, this dialog stole focus, and the ENTER key chose ALLOW (or SPACE chose disallow) before I even noticed the dialog had appeared.

jsejcksn commented 4 years ago

I opened a workspace and did something like COMMAND+P to open a file. In the middle of typing, this dialog stole focus, and the ENTER key chose ALLOW before I even noticed the dialog had appeared.

Another problem with modal dialogs: stealing focus and accidental input (Enter, Escape, Spacebar, etc.)

fortinmike commented 4 years ago

I agree that this popup does nothing to enhance security. Most if not all people will just click "Allow" (it's well-known that users dismiss unsolicited popups as quickly as they can) and those who click disallow, probably by accident, will get a "broken" editing environment without linting, which can be problematic in a team as some pointed out. When you think about it, all dependencies must be trusted and imply some level of security risk, but I sure am glad I don't get a modal popup for each and every one of those.

I'd also vote for this to be removed ASAP until a better, less obnoxious and actually security-enhancing feature is implemented.

y0nd0 commented 4 years ago

Yes this is annoying. Just use it. Priority: Try to use the local in node_module of the workspace and if not the globally installed one or nothing. And if you can't decide it for now, display a vscode notification. This notifications has the same features as the dialog. But I prefer a silent solution. Just use the workspace eslint or global one. Display an information about it if you want.

Please no options like "always allow" or something. No dialog at all. This should not annoy or confuse team mates. Everyone should use a linter. I don't understand why is this blocked by default? What is the reason for that dialog?

rickbeerendonk commented 4 years ago

I made a search/replace in 171 files and now I have two options:

ghost commented 4 years ago

very annoying popup very time

dashersw commented 4 years ago

Please remove this. If I deliberately install an ESLint in a project, I for sure want to use that version. Why should I ever use another version?

fortinmike commented 4 years ago

Since initially commenting on this, I accidentally dismissed the dialog a dozen times (I don't know what button I accidentally triggered) because I was typing when the dialog unexpectedly popped up (I think I had just quick-opened a file in a different project in my workspace, or opened one of those files in find results).

Did I disable eslint for part of my project, or did I keep it enabled when this occurred? I don't know. This must be removed ASAP, in my opinion.

ryanblock commented 4 years ago

@dbaeumer really feels like folks are trying to tell you this is not working for them. Will you continue to hold the position that you won't fix this glaring usability issue until eventually VS Code itself supports trusted workspaces? (Is that even something that could feasibly ship this year?)

For everyone else: you can opt out of this by:

bduffany commented 4 years ago

Thanks @ryanblock for the suggestion to revert back to 2.1.6! Didn't know it was possible to install a previous version.

dbaeumer commented 4 years ago

I will not be able to revert this back due to the fact that this is a publicly know security vulnerability (see https://github.com/microsoft/vscode-eslint/issues/1012#issuecomment-658920804)

Having an Always Allow button can only come with a good UI indication that ESLint is trusted in general which goes along the lines of having a good UI for trusted workspace. So instead of me coming up with a custom solution I decided I better support VS Code as good as possible. To my knowledge the VS Code team already started working on this in August. I also committed to adopt this even if it is only proposed API and might need extra work on my side to improve the situation for ESLint users.

I started to look into using a notification instead of a dialog but I couldn't come up with a good flow so far. Major reason is that the VS Code API doesn't allow me to (a) cancel a notification or (b) make a hidden notification re-appear. Ideas / suggestions are welcome here.

bduffany commented 4 years ago

@dbaeumer I think a lot of people are going to revert back to 2.1.6 or possibly even uninstall vscode-eslint simply out of pure frustration (evidence: 4 people have put their thumbs-up on the comment made by @ryanblock only yesterday). For me personally, I reverted back to 2.1.6 because I kept switching between different packages inside a monorepo, where each package was using a different version of eslint, and I kept seeing the warning, which just got to be too annoying and actively thwarting my productivity.

IMO a temporary "Always Allow" button would be a good solution to avoid people uninstalling the extension (or worse, reverting to a previous version, which will result in people reporting bugs that have already been fixed, not realizing they are still pinned to the old version). Especially since none of us really knows how long it's going to take the VS Code team to implement the trusted workspaces feature.

Even though you committed to using their system, you aren't breaking that commitment just by adding a temporary "Always Allow" button, IMO.

jsejcksn commented 4 years ago

Major reason is that the VS Code API doesn't allow me to (a) cancel a notification or (b) make a hidden notification re-appear

The modal dialog is much more problematic than these listed concerns.

dbaeumer commented 4 years ago

The problem with the Always Allow is that I need to design something that is acceptable from a security perspective. It is not only about adding a button. Since VS Code needs to do the same I simply want to avoid the double effort.

The modal dialog is much more problematic than these listed concerns.

The concerns are not the API the concerns are the problems that raise with this. I will add the changes I already did to a branch. @jsejcksn may be you want to contribute if you have a good idea?

jsejcksn commented 4 years ago

may be you want to contribute if you have a good idea?

@dbaeumer I shared an idea earlier in this conversation about using a notification instead of a modal dialog, but you dismissed it.

Are you saying that you have changed your mind and now would like for someone to contribute a PR that replaces the modal dialog with a notification like the one I mentioned?

dbaeumer commented 4 years ago

@jsejcksn I did another attempt at it but it is still not nice. The easy thing about the dialog is that there is one question at a time and the user can't open another file when the dialog is present. This is completely different with notifications and the way how the API works. You might end up with n stacked notification asking for n different eslint libraries. In addition quite some management code needs to be written to ensure that you don't see two notifications for the same library. The solution is not to simply remove the modal flag. In addition I still think that there is a high change the users miss the notification. That is why in my current attempt the notification is behind a setting.

bduffany commented 4 years ago

@dbaeumer I think I understand where you're at now. You could implement your own "Always Allow" button, but the solution would essentially be identical to what the VS Code team is already trying to implement.

But I would hazard a guess that the VS Code is trying to implement something much more comprehensive, with UI controls for trusted workspaces, etc., and it's going to take them a while.

It seems to me like all you would need to do is keep a cache like { '$workspaceFolder': '$hasClickedAlwaysAllow' }, then check if (!hasClickedAlwaysAllow[$workspaceFolder]) showModalDialog().

Would it be more complex than that? I'm not an expert in VS Code by any means so I'm not sure if the concept of workspaceFolder is even applicable, or whether VS Code even lets you keep a cache like that.

This may have its own security issues (e.g. what happens if you replace a workspace folder with a different workspace?), but IMO this would be strictly more secure than people reverting to version 2.1.6 out of pure annoyance.

FWIW, I don't think notifications are the right approach, for the reasons you mentioned (stacked notifications, people ignoring them, etc.)

DLehenbauer commented 4 years ago

I avoided being prompted for each new folder in a mono repo by pointing the plugin at a specific ESLint instance:

    "eslint.nodePath": "packages/common/config/eslint-config/node_modules/",      // Avoid prompts when detecting ESLint installation
dbaeumer commented 4 years ago

@bduffany do I understand you correctly that what you are suggesting is for all eslint lbraries in one workspace folder. It is not about allowing eslint for all workspace folders including future once.

ranyitz commented 4 years ago

Thanks @ryanblock I'm going to revert to 2.1.6.

@dbaeumer In case someone successfully inserted a malicious version of eslint to my node_modules directory I would most probably won't know about it and press allow anyways. Therefore I don't see how this increases the security. It just resulting with more manual clicking and like most people said, you can easily ignore the popup and you'll get a problem which is not clear how to solve (eslint is now disabled for this project).

ranyitz commented 4 years ago

Adding a visual explanation for the workaround (Installing the version of this plugin that didn't include this change)

image

rjgotten commented 4 years ago

@ranyitz In case someone successfully inserted a malicious version of eslint to my node_modules directory I would most probably won't know about it and press allow anyways.

Doesn't even have to be the case that it's the eslint module itself which is malicious. Could be any plugin loaded by the eslint configuration and in case of the configuration being a .eslintrc.js file -- i.e. it being full on executed JavaScript -- it could even be the configuration file itself.

ramigs commented 4 years ago

as suggested by @ryanblock reverting to 2.1.6 solved it for me

martintasker commented 4 years ago

I've just downgraded eslint to 2.1.6.

I'm really sad about this. VS Code is a fabulous IDE, the eslint plugin is brilliant tool. But this UX since mid-July is the first really silly thing that's happened since I've started using Code.

Just to add another dimension of pain, I have very poor eyesight, which means on the one hand that I particularly appreciate VS Code's beautiful design, and on the other hand that this dialog is doubly annoying because it's just plain old hard-to-read native stuff.

Please, @dbaeumer, can you persuade whoever needs persuading in the VS Code project, that all reasons for the post-July behaviour are irrelevant compared with the regression in usability.

Laev commented 4 years ago

as suggested by @ryanblock reverting to 2.1.6 solved it for me

This also helped me

dbaeumer commented 4 years ago

@martintasker the VS Code team started work to come up with a better UI that I can reuse. See my comments here https://github.com/microsoft/vscode-eslint/issues/1012#issuecomment-675523469 explaining that I am unable to simply revert that back.

martintasker commented 4 years ago

@martintasker the VS Code team started work to come up with a better UI that I can reuse.

Sounds good. Thanks @dbaeumer.

dpistoleKUBRA commented 4 years ago

This one bit me. I've lost 3 days of productivity trying to move eslint config around to figure out why I would get way more typescript lint errors from my npm lint script (which uses ./node_modules/.bin/eslint) compared to the code editor.

I assume at some point in the past I clicked something other than Allow on the prompt out of frustration at its frequency. I was getting some errors (not nearly as many as from npm lint though, which uses a local eslint) so maybe it was falling back to some globally installed (and likely older version of) eslint or something.

The fact that it's not in the settings JSON made it extra hard to find the solution. Ultimately the ESLint: Reset Library Decisions step outlined here fixed it for me.

Love VS code, hate that prompt and am glad you guys are working on a better solution. Thanks for your efforts.

dbaeumer commented 4 years ago

For others that hit this: there is a visual indication in the status line that ESLint is blocked.

capture
Gameghostify commented 4 years ago

Had to revert to 2.1.6 as well

It would be awesome if there was a flag to opt-out

Being opt-in by default is a good thing as it's for security, but letting us opt out with a setting like eslint.disableLibraryDecisions is something that feels like it's missing at this point

rjgotten commented 4 years ago

letting us opt out

... would open a security hole. A malicious project could include that setting in a .vscode/settings.json file local to the project, override your choice and force their doctored ESLint to be used directly.

bduffany commented 4 years ago

letting us opt out

... would open a security hole. A malicious project could include that setting in a .vscode/settings.json file local to the project, override your choice and force their doctored ESLint to be used directly.

@rjgotten I don't see your point -- if you downloaded a malicious project, you are already screwed. Running yarn install / yarn start within that project could do god knows what to your computer. If someone got you to download their malicious project, why the heck would they use ESLint as the attack vector? They could just execute arbitrary code using the programming language of their choice.

dbaeumer commented 4 years ago

@bduffany the problem is that you can create a Git repository with all node_modules included. In this case the user simply needs to open VS Code on it and the ESLint extension executes the code without the user knowing.

In the yarn install case the user needs to type an install command in a terminal.

bduffany commented 4 years ago

@dbaeumer Does not every other extension run into this problem? What about Prettier, TypeScript, etc.?

dbaeumer commented 4 years ago

I can't talk about Prettier. TypeScript doesn't since by default they use the TS version that ships with VS Code. If you want to use a workspace version you get prompted and need to confirm that.

rjgotten commented 4 years ago

if you downloaded a malicious project, you are already screwed.

That's pretty much the point I've been making before as well. There are way too many attack vectors possible to cover them all.

Even when a project doesn't pre-include a malicious node_modules folder and the user has to actively perform npm install it's still possible to be screwed over with a malicious module. Even if you vet the package.json file. Because most people will still forget to check the .npmrc dot-file and that can reconfigure the repository URL and point it to a compromised repository.

The point of contention then becomes; do you want to lessen that attack-surface by sacrificing usability? Is the trade-off worth it, even if it would still leave a hole a mile-wide.

And there's something to be said for both sides in that debate.

Realistically, the only way to make this 'safe' would be to only load the ESLint module from node_modules after you've computed a checksum hash over its contents and those contents match a known version. That means this VSCode extension would never be forward compatible and you'd always need an update to support newer ESLint releases. Well; unless you could get hold of those content hashes out-of-band. Maybe by querying the NPM servers?

(Wouldn't it have been nice if the whole node_modules and NPM architecture would've been put together with some actual architectural forethought for security and authentication, and have support for things like signed packages as first-class citizens?)

trideepgogoi commented 4 years ago

This might seem like a minor inconvenience, but for folks that work on multiple projects and a large code base this is turning out to be a nightmare. Even If I accept the security issues around this, the dialog is completely unintuitive. I would guess that most folks are simply going to click the "Allow" Button anyways since there's no indication of why you would want to click Do not allow. And if you click on Do not allow, not having ESlint run create its own headache. This isnt really solving the security issue (a real fix would mean disabling all 3rd party plugins). If the purpose is to inform the user that can be done via a warning. Asking the user a seemingly random question is 100% unintuitive. This is compounded by the fact that most folks encountering this have now probably spent days trying to fix their own configs before finally stumbling onto this Issue.

javierguzman commented 4 years ago

I have a workspace with two folders inside. Each folder has its own "node_modules". Is there a way to tell ESLint to use the respective eslint within the respective node_modules depending what file I am viewing? So far, let's say I choose eslint uses folder1 node_modules, even if I do stuff in folder2, eslint will keep using folder1's node_modules which I think it does not make sense.

Thanks

dbaeumer commented 4 years ago

The extension uses the installed eslint version closest to the file you are validating using ESLint's lookup rules. Can you provide me with a GitHub repository that demos what you are seeing. It should work the way you described it.

javierguzman commented 4 years ago

I have tried to reproduce it but I have been unsuccessful. I have reset everything now and I have observed I get this output:

[Info  - 17:55:20] ESLint library loaded from: /blabla/application/client/node_modules/eslint/lib/api.js
[Info  - 17:55:26] ESLint library loaded from: /blabla/application/server/node_modules/eslint/lib/api.js

I guess once the eslint plugin has the library loaded it switches between them on the fly depending on whether you are editing a file from client folder or from server, right?

Thank you in advance.

dbaeumer commented 4 years ago

I talked to the VS Code team about trusted workspaces and unfortunately the feature will not be ready this milestone. So I invested quite some time to improve the situation in ESLint knowing that this is throw away work. Here is how the new model works:

capture capture capture

A test version of the new extension is here: https://github.com/microsoft/vscode-eslint/releases/tag/2.1.9-insider

I would like to ask people from the community to give it a try especially those who reverted back to 2.1.6. Please add comments to this issue with your feedback.

infacto commented 4 years ago

What is this dialog for? Why allow / disallow? Just use the eslint of the workspace or if not exist, use global. Other settings could be configured in the settings json of vscode. I do not understand the problem. Simply use the eslint of node_modules from my project. Every team mate should use the workspace one. No options / dialog.