ScriptTiger / Unified-Hosts-AutoUpdate

Quickly and easily install, uninstall, and set up automatic updates for any of Steven Black's unified hosts files.
https://scripttiger.github.io/more/
MIT License
312 stars 25 forks source link

Script closes cmd.exe #35

Closed ohadschn closed 4 years ago

ohadschn commented 4 years ago

Currently the user's cmd.exe is closed when the script ends. This is intrusive and can be annoying if for example you had other stuff you wanted to do in that terminal (you set up environment variables, you wanted to look at command output, you needed the command history, etc). This also interferes with debugging, I can't look at your script's logs or copy them because the windows immediately closes.

I suggest you always use exit /b so that the user's cmd.exe remains open.

ScriptTiger commented 4 years ago

Debugging is easily enough written into the script itself, which is one of the reasons why I include a feature to stop automatic script updates, if you have customized your script and don't want it to be overwritten automatically. With my other command-line utilities you'll see I do have a preference to use exit /b where it makes sense. However, for this particular script for the average person running this script inside of an already open command prompt and not running it stand-alone can actually be quite dangerous. Remember this script is running with administrative permissions and accessing sensitive system files. If the hosts file were to break in any way, some people may not be able to log into their system at all. For this reason I purposely designed the script with the assumption it would be run stand-alone, with the average person in mind.

I may review this issue at a later date, but that's my stance on the matter for the moment.

ohadschn commented 4 years ago

So your argument is that an admin cmd window is dangerous, so you're deciding to close it for the user? With respect, this is the user's machine - not yours. If I opened a cmd, I think I have the right to decide when I want to close it...

This is also how any other utility works, admin or not. Can you point out a single tool / script that closes the current cmd window after you run it? Imagine if schtasks behaved like that, how would you use it? How will someone be able to use your script as part of a bigger script (technically it will be possible by starting a new process, but that just add complexity, you'd need to redirect logs, etc)?

BTW, debugging is not easy, because the script immediately closes and you don't get the chance to see the error. For example in the other issue I opened about the default editor not opening, I couldn't copy the log because the minute I closed the message box, the console was closed (and while the message box was open I couldn't interact with the console since it was modal).

ScriptTiger commented 4 years ago

The Windows command-line interpreter is the same language in a command prompt as it is in a batch file, which is one of the reasons I prefer to use it for instruction purposes as students can easily see its actions live in the window as well as use what they learned and put it into a script file. For those experienced enough to know the language, it would make no difference whether they incorporate their debugging in the window or in the script file itself, which is how even I myself debug it. If I need to keep it open for whatever reason to collect information, a custom edit can be done quickly for that purpose. But you could also just as easily write a quick edit to dump what you need to a file without needing the window to remain open.

Trust me, I completely understand where you are coming from, but allowing the script to remain open after it is run may encourage someone to think they can just run it again real quick if they forgot to set a scheduled task or realize they set the wrong blacklist. However, this can have seriously detrimental effects because this script has a certain level of complexity and numerous volatile and sensitive environmental variables used for controlled iterations that can easily go awry and cause massive file system damage as well as real physical damage to one's processor and cause physical power drain if any of those variables are misconfigured. Unless it is absolutely necessary to expose said variables to an unknown environment, it is preferred to take every precaution possible to ensure a pristine run-time environment.

ohadschn commented 4 years ago

If environment variables are your concern you can simply use SETLOCAL.

And again, the fact is that no script and no tool that I know of, regardless of how dangerous they can be, will forcibly close a cmd window that a user has opened. I suppose one compromise would be a CLI option to keep it open (e.g. -noclose)

ScriptTiger commented 4 years ago

I do like the switch option idea, although it would also have to come with appropriate words of caution. I'll keep this open and review it again later. Thanks for your ideas!

As for using SETLOCAL, I entirely agree with you. However, I have to admit that 99.9% of this script was solely written by myself and I don't have a high degree of trust in myself that all of the localizations have been properly implemented, at least a high enough degree of trust to roll this out to the hundreds of people currently using it and possibly put them at risk. Another reason for the safeguards is also protecting users from my own inadequacies. I seriously need to take time to go through everything more closely and review everything, but I just don't have the time at the moment. PRs always welcome!

ohadschn commented 4 years ago

I'll be honest with you, I just ran the script once and I'm happy, not really enough to warrant a PR from my end, just sharing my feedback :)

I do think you are a bit too paranoid - nobody is perfect, including Microsoft developers (I know because I'm one of them), and yet everybody is keeping their console windows open...

ScriptTiger commented 4 years ago

I will take that point, I am paranoid :P I have to keep my paranoid standards in good standing with those of @StevenBlack and his community since this project is basically a direct offshoot.

Also, I don't know if maybe you might want to edit or delete your comment... A huge chunk of what we do is block Microsoft telemetry, Bing ads, Bing analytics, etc., etc., same as Google. I don't know if maybe making that comment might put you in bad standing, or possibly even have some legal liability, as a company representative. And that's just my concern for you on a personal level, because I surprisingly have a lot of Microsoft developers/Redmond residents following my work, but most of them stay quiet and prefer communicating via e-mail :P

ohadschn commented 4 years ago

LOL I appreciate the concern, but I am in no shape or form an MS representative. I just work there, my opinions here are my own :)

ScriptTiger commented 4 years ago

I am glad you made that disclaimer... They're watching :P

spirillen commented 4 years ago

You are so paranoid @ScriptTiger :rofl:

ScriptTiger commented 4 years ago

I have just addressed this issue in 1.31 but have not yet released it, pending your confirmation. Just like previously, please manually download the developmental script (https://raw.githubusercontent.com/ScriptTiger/Unified-Hosts-AutoUpdate/master/Hosts_Update.cmd) to replace your current script, right-click and edit the downloaded script file to change set V=1.31 to set V=1.30, and then execute it under your test conditions.

The magic switch is /DFC ("don't force close"), so Hosts_Update.cmd /DFC. Also, /DFC MUST be the first argument, before URL and compression level if those are also present (i.e. Hosts_Update.cmd /DFC https://www.myurl.com 9).

Let me know if everything goes smoothly and as expected, and then I'll release it.

ohadschn commented 4 years ago

Works great 👍

ScriptTiger commented 4 years ago

Thanks!

Case closed.