Skip to content

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

Open
ejtejada wants to merge 12 commits intojlambert360:masterfrom
ejtejada:master
Open

[Feature Request] Avoid Running Scripts from Directly Online#5
ejtejada wants to merge 12 commits intojlambert360:masterfrom
ejtejada:master

Conversation

@ejtejada
Copy link

@ejtejada ejtejada commented Jun 3, 2020

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

@jlambert360 jlambert360 requested a review from jsj1027 June 4, 2020 00:32
@ejtejada
Copy link
Author

ejtejada commented Jun 5, 2020

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

@jsj1027
Copy link
Collaborator

jsj1027 commented Jun 9, 2020

@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
Copy link
Author

@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
Copy link
Collaborator

jsj1027 commented Jun 12, 2020

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
Copy link
Author

ejtejada commented Jun 19, 2020

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
Copy link
Author

ejtejada commented Jun 20, 2020

@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 ejtejada requested a review from jsj1027 August 31, 2020 22:40
@ejtejada
Copy link
Author

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

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

Successfully merging this pull request may close these issues.

2 participants