Skip to main content
Code Review

Return to Answer

I missed another literal use of hard-coded program name
Source Link
Toby Speight
Toby Speight
  • 88.4k
  • 14
  • 104
  • 327
help() {
    local tab=$'\t'
    cat <<END
rwog$0 - run without groups
Runs a shell as if you weren't in certain supplementary groups.
Good for pretending that you're not a support user.
Usage example:
${tab}$0 [(-h|--help)] [group ...]
Options:
${tab}-h or --help: Displays this information.
END
}

help() {
    local tab=$'\t'
    cat <<END
rwog - run without groups
Runs a shell as if you weren't in certain supplementary groups.
Good for pretending that you're not a support user.
Usage example:
${tab}$0 [(-h|--help)] [group ...]
Options:
${tab}-h or --help: Displays this information.
END
}

help() {
    local tab=$'\t'
    cat <<END
$0 - run without groups
Runs a shell as if you weren't in certain supplementary groups.
Good for pretending that you're not a support user.
Usage example:
${tab}$0 [(-h|--help)] [group ...]
Options:
${tab}-h or --help: Displays this information.
END
}
Recommend a comment to document name=group assumption
Source Link
Toby Speight
Toby Speight
  • 88.4k
  • 14
  • 104
  • 327

If you did mean to compare against the user name (e.g. you know your configuration always creates a per-user group sharing the same name), then it's a good idea to add a comment documenting that assumption, as it might not be valid on another system you copy this to.

#No need to grep and sed in a pipeline Sed is perfectly capable of selecting lines:

#No need to grep and sed in a pipeline Sed is perfectly capable of selecting lines:

If you did mean to compare against the user name (e.g. you know your configuration always creates a per-user group sharing the same name), then it's a good idea to add a comment documenting that assumption, as it might not be valid on another system you copy this to.

#No need to grep and sed in a pipeline Sed is perfectly capable of selecting lines:

Source Link
Toby Speight
Toby Speight
  • 88.4k
  • 14
  • 104
  • 327

#It's not an error to ask for help When -h is passed, the help() function shouldn't exit with non-zero status, because it's done what was asked for.

Conversely, when we pass no arguments, we should exit with non-zero, and we should write the message to the standard error stream.

I'd write that as

help() {
    local tab=$'\t'
    cat <<END
rwog - run without groups
Runs a shell as if you weren't in certain supplementary groups.
Good for pretending that you're not a support user.
Usage example:
${tab}$0 [(-h|--help)] [group ...]
Options:
${tab}-h or --help: Displays this information.
END
}

And then use it as

# Just use the command directly instead of testing $?
if ! ARGS=$(getopt -o "h" -l "help" -n "rwog" -- "$@");
then
    help >&2
    exit 1
fi

and

    case "$1" in
        -h|--help)
            help
            exit 0
            ;;

Elsewhere in the code, diagnostic messages are printed to standard output; these should all be redirected to &2.

#Checking more exit statuses We depend on tools such as xargs and sort that might not be present. Consider letting Bash check some statuses, but make sure you know which commands this will not check:

set -e

Also consider getting Bash to check that you're not using unset variables:

set -u

You'll want to use expansions such as ${foo:-} when you need to avoid this checking.

#Quote your variable expansions Just because you've checked that this is being run by root (perhaps overkill - is it sufficient to be any user that can run capsh?), doesn't mean that they can't do something unexpected. So write "$@", not $@:

for group in "$@"; do

#Don't compare a group against a user

if [[ "$group" == "$SUDO_USER" ]]; then

Did you mean "$SUDO_GID" there? Also, there's little point using both [[...]] and quotes - I'd stick to the portable [...] here.

#No need to grep and sed in a pipeline Sed is perfectly capable of selecting lines:

dropped_capabilities=$(capsh --print | sed -n -e 's/Bounding set =//gp')

#Take care with $PATH Even though your site's sudo policy should constrain PATH, it may be a good idea to set a specific PATH at the start of the script and/or use fully-qualified pathnames for executables.