mrlacey / CommentLinks

Add links between files in code comments
https://marketplace.visualstudio.com/items?itemName=MattLaceyLtd.CommentLinks
MIT License
31 stars 5 forks source link

Request: Disable link:run> syntax by default for security reason #53

Closed skhrshin closed 1 year ago

skhrshin commented 1 year ago

I think link:run> syntax is very danger because the user may read a source code which is written by some attackers. The user is not fully understanding the exact behavior of the written command and just clicking the link invokes the command without any warning. Is it possible to disable this feature by installation default and only allow if the extension is configured as so?

mrlacey commented 1 year ago

If someone is installing 3rd party extensions and downloading unknown code with the ultimate intention of running it, there are already far more security risks than running commands this way.

If it was disabled by default, what do you propose as the behavior for enabling it and/or causing awareness of it, if not showing the button to execute it?

skhrshin commented 1 year ago

If someone is installing 3rd party extensions and downloading unknown code with the ultimate intention of running it, there are already far more security risks than running commands this way.

Yes, installing 3rd party extension is a danger operation, so everyone must be careful for doing it. I think clicking a link in a editor window should be a safe operation, like when you click a link in a web browser. These two are completely different.

If it was disabled by default, what do you propose as the behavior for enabling it and/or causing awareness of it, if not showing the button to execute it?

How about placing a check box to enable the feature at the area showing information of selected extension in the Manage Extensions dialog box?

I don't know much about what extensions can do, so sorry if it was not possible to implement under the current mechanism of Visual Studio. Another idea is to have a configuration file somewhere under the user's home directory.

mrlacey commented 1 year ago

I can store the setting and provide a way to offer the configuration. I'm more interested in understanding why you think this is an issue and what sort of commands you've seen included in source files and are worried about executing.

What would you consider a sufficient solution?

Is it enough to have a warning before the first time such a command is used? Or, given that people don't read dialogs, does it need to be a more explicit opt-in? In that few people also go looking for settings, I want the functionality to be easily discoverable. Do you have thoughts about this?

Is opting into this functionality once enough? What if they don't use it for an extended period of time, should they have to opt-in again? They may have forgotten that they installed the extension, or what it does, and the potential for irreversible consequences.

Or is it a concern that someone may accidentally or unintentionally click on something that might have non-trivial consequences?

All of this really begs the question as to why anyone would ever include such a link in the first place.

This is all a theoretical situation after someone has installed an extension that adds the capability to open links or run commands that are specified in the comments of source code. To not make this possible seems limiting.

As an attack vector for someone wanting to spread malicious code, there are much easier ways to reach a much larger potential audience.

Is this perceived security concern a barrier to you using the extension or recommending it to others?

Do you know many developers who are likely to click buttons in the editor just to see what they will do, which is driving this concern?

Or is this just a concern that I might suffer negative consequences if someone did run a command they regretted and then blamed me?

skhrshin commented 1 year ago

I'm more interested in understanding why you think this is an issue and what sort of commands you've seen included in source files and are worried about executing.

Here is an example.

A file named something_very_useful.bat is at the same directory the source file is located under.

set SPECIFICDIR=garbage_folder
del /f /s /q %USERPROFILE%\%SPEClFlCDIR%

In the source file, there is a link like below:

// link:run>"cmd.exe /c something_very_useful.bat"

This link actually removes all files under the user's home directory, because the batch file sets an environment variable SPECIFICDIR but the one actually specified at del command is SPEClFlCDIR and it is expanded to an empty string.

Is it enough to have a warning before the first time such a command is used? Or, given that people don't read dialogs, does it need to be a more explicit opt-in? In that few people also go looking for settings, I want the functionality to be easily discoverable. Do you have thoughts about this?

I think it should be an explicit opt-in. To make people who have never visited the configuration dialog know about this feature, how about showing a dialog which explains that there is the option to turn the feature on, at the first time (or every time with having "Do not show this again" check box) disabled link to run command is clicked?

Is opting into this functionality once enough? What if they don't use it for an extended period of time, should they have to opt-in again? They may have forgotten that they installed the extension, or what it does, and the potential for irreversible consequences.

That is a good point I haven't ever noticed. I would be nice if an expiration time to revert the feature option back to "Disabled" can be configured within a range like 1 day to 30 days.

Or is it a concern that someone may accidentally or unintentionally click on something that might have non-trivial consequences?

That is also possible. Even if the user chooses to enable the option, it would be nicer if the user can also choose whether to show a warning dialog or not.

Is this perceived security concern a barrier to you using the extension or recommending it to others?

Yes, exactly.

Do you know many developers who are likely to click buttons in the editor just to see what they will do, which is driving this concern?

If there is a URL pasted in a comment, which is explained to be a reference to some document, I will ctrl+click it to open with my web browser with few concerns about security. I think the link button is almost same.

mrlacey commented 1 year ago

So, to sum up:

mrlacey commented 1 year ago

v2.16 (releasing shortly) will require confirmation of responsibility for executing commands before they are run. A setting in the options pane will allow the resetting of this flag if so desired. Technically the license already absolves me of any responsibility for unintended consequences, but hopefully, this will avoid any accidental, unintended consequences or concerns about the safety of developers considering using this extension.