updater.sh/prefsCleaner.sh: Check for root and abort #1651
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Check if running as root and if any files have the owner/group as root|wheel. Abort on both. Should (hopefully) prevent stuff like: arkenfox#1587 Discussion: arkenfox#1595
mhmdanas
reviewed
Apr 1, 2023
mhmdanas
reviewed
Apr 1, 2023
Co-authored-by: Mohammed Anas <triallax@tutanota.com>
mhmdanas
reviewed
Apr 6, 2023
mhmdanas
reviewed
Apr 6, 2023
| elif [ -n "$(find ./ -user 0 -o -group 0)" ]; then | ||
| printf 'It looks like this script was previously run with elevated privileges, | ||
| you will need to change ownership of the following files to your user:\n' | ||
| find . -user 0 -o -group 0 |
There was a problem hiding this comment.
Is there a way to avoid running find . -user 0 -o -group 0 twice?
There was a problem hiding this comment.
You could do:
pc_ROOT_ID="$(find . -user 0 -o -group 0)"
...
elif [ -n "${pc_ROOT_ID}" ]; then
...
printf '%s\n' "${pc_ROOT_ID}"
...
but I think this makes the code more indirect for no real gain.
mhmdanas
reviewed
Apr 6, 2023
Co-authored-by: Mohammed Anas <triallax@tutanota.com>
|
good idea, thanks! |
rusty-snake
reviewed
Apr 22, 2023
| @@ -10,7 +10,7 @@ | |||
|
|
|||
| # Check if running as root and if any files have the owner/group as root/wheel. | |||
| if [ "${EUID:-"$(id -u)"}" -eq 0 ]; then | |||
| printf 'You should not run this with elevated privileges (such as with doas/sudo).\n' | |||
| printf 'You shouldn't run this with elevated privileges (such as with doas/sudo).\n' | |||
There was a problem hiding this comment.
This needs to be escaped/quoted, otherwise it is a syntax error breaking the script.
There was a problem hiding this comment.
Suggested change
| printf 'You shouldn't run this with elevated privileges (such as with doas/sudo).\n' | |
| printf 'You shouldn'"'"'t run this with elevated privileges (such as with doas/sudo).\n' |
dngray
reviewed
Apr 24, 2023
|
|
||
| ## special thanks to @overdodactyl and @earthlng for a few snippets that I stol..*cough* borrowed from the updater.sh | ||
|
|
||
| ## DON'T GO HIGHER THAN VERSION x.9 !! ( because of ASCII comparison in update_prefsCleaner() ) | ||
|
|
||
| # Check if running as root and if any files have the owner/group as root/wheel. | ||
| if [ "${EUID:-"$(id -u)"}" -eq 0 ]; then | ||
| printf 'You shouldn't run this with elevated privileges (such as with doas/sudo).\n' |
There was a problem hiding this comment.
btw, this is still incorrect and broken. You don't want single quotes here. As you will get this error:
line 13: syntax error near unexpected token `('
line 13: ` printf 'You shouldn\'t run this with elevated privileges (such as with doas/sudo).\n''
Suggested change
| printf 'You shouldn't run this with elevated privileges (such as with doas/sudo).\n' | |
| printf "You shouldn't run this with elevated privileges (such as with doas/sudo).\n" |
Really ought to run these shell scripts through Shellcheck as well and fix the other errors.
| if [ "${EUID:-"$(id -u)"}" -eq 0 ]; then | ||
| printf 'You shouldn't run this with elevated privileges (such as with doas/sudo).\n' | ||
| exit 1 | ||
| elif [ -n "$(find ./ -user 0 -o -group 0)" ]; then |
There was a problem hiding this comment.
This find check will cause issues when you want to run the script in another directory (not the current directory) ie option -p
dngray
reviewed
Apr 24, 2023
| printf 'You shouldn't run this with elevated privileges (such as with doas/sudo).\n' | ||
| exit 1 | ||
| elif [ -n "$(find ./ -user 0 -o -group 0)" ]; then | ||
| printf 'It looks like this script was previously run with elevated privileges, |
There was a problem hiding this comment.
Suggested change
| printf 'It looks like this script was previously run with elevated privileges, | |
| printf "It looks like this script was previously run with elevated privileges, | |
| you will need to change ownership of the following files to your user:\n" |
Don't use single quotes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Check if running as root and if any files have the owner/group as root|wheel. Abort on both.
Should (hopefully) prevent stuff like: #1587
Discussion: #1595
This does not attempt to correct the permissions, just notify the user
and abort.
If I followed #1587 right.
The original person didn't realize that it appended and subsequent
people tried to run with elevated privileges. This should at least fix
the second.