jlambert360 / FPM-Installer

THIS SCRIPT IS OUTDATED PLEASE USE THE APPIMAGE REPO INSTEAD.
https://github.com/jlambert360/FPM-AppImage/releases/latest
8 stars 6 forks source link

[Feature Request] Avoid Running Scripts from Directly Online #5

Open ejtejada opened 4 years ago

ejtejada commented 4 years ago

Hello Everyone, This script was awesome, and allowed me to build Ishiiruka for netplay, so thank you! However, the current way to check if the script is up to date is dangerous. I would never suggest anyone run a script grabbed from curl, as you cannot read it before you run it. Thus, I changed the check you implemented. Instead of disallowing the script to run from a downloaded instance, I grab the md5sum of the online master, then compare it to the current one for the existing setup file. This safely allows a user to just ./setup And makes sure they are up to date. However, doing this requires md5sum become a dependency and the shell set to bash (not dash) explicitly. This should causes no issues, but was needed for my string comparison behavior on line 16. I tested these changes to script and it compiled no problem. I also explicitly stated the dependencies for Ubuntu in the README. Have a great day, ~Edgar

ejtejada commented 4 years ago

@jsj1027 Should I close this pull request, make your suggested changes, and the open a new pull request?

jsj1027 commented 4 years ago

@jsj1027 Should I close this pull request, make your suggested changes, and the open a new pull request?

No feel free to just continue the changes on this pull request

ejtejada commented 4 years ago

@jsj1027 Ok, I reverted all suggested changed and testing using the default shell in Ubuntu (not bash) and it worked on my own branch.

If you do accept this pull request, wait a few minutes for github to update the raw file and run curl -Ls https://github.com/Birdthulu/FPM-Installer/raw/master/setup && ./setup Just fo verify it runs on your master. ~Edgar

jsj1027 commented 4 years ago

So im liking this, but when testing it I ran into an issue. The created setup file isnt created as an executable. I tested it by running curl -Ls https://github.com/Birdthulu/FPM-Installer/raw/master/setup && ./setup And

curl -Ls https://github.com/Birdthulu/FPM-Installer/blob/3fd45d8e29ed29cdb9410d10a8bd2e11215a9b78/setup && ./setup I think a chmod +x setup may be needed unless there is some other work around.

Also I noticed when testing with the first command, that a setup file isnt created. Which is weird because it work basically the same way as before right?

ejtejada commented 4 years ago

I think curl doesn't behave as expected if the file doesn't already exist. See: https://stackoverflow.com/questions/13735051/how-to-capture-curl-output-to-a-file

One fix is to explictly declare where curl should save to curl -Ls https://github.com/Birdthulu/FPM-Installer/raw/master/setup -o setup or we can use wget, which I know is slow wget https://github.com/Birdthulu/FPM-Installer/raw/master/setup

Also, I see no way around having to chmod +x setup if the file doesn't already exist without the correct permissions.

ejtejada commented 4 years ago

@jsj1027 I have made all of your suggested changed and tested on my own branch. Try wget https://github.com/ejtejada/FPM-Installer/raw/master/setup && chmod +x setup && ./setup In some folder where setup does not exist, as well as where it does exist. In both cases it should exit, since my hash doesn't yet match Birdthulu's version. If you accept the pull request, please wait a few minutes and try

wget https://github.com/Birdthulu/FPM-Installer/raw/master/setup && chmod +x setup && ./setup both in a folder with setup and in a folder with no setup file. It should (hopefully) work in both cases. ~Edgar

ejtejada commented 4 years ago

@jsj1027 Hello again, I know you were all busy with updates to P+. Are there any more needed changes for this pull request?