Shellcheck: rm -rf warning is disabled by adding parameters to rm, rather than shellcheck

[markscsmith]

For bugs

• Rule Id (if any, e.g. SC1000): SC2114
• My shellcheck version (shellcheck --version or "online"): 0.4.5
• [X] I tried on shellcheck.net and verified that this is still a problem on the latest commit
• [ ] It's not reproducible on shellcheck.net, but I think that's because it's an OS, configuration or encoding issue

For new checks and feature suggestions

• [ ] shellcheck.net (i.e. the latest commit) currently gives no useful warnings about this
• [X] I searched through https://github.com/koalaman/shellcheck/issues and didn't find anything related

Here's a snippet or screenshot that shows the problem:

#!/bin/bash
rm -rf /usr /local/someapp/bin --

Here's what shellcheck currently says:

$ shellcheck myscript
No issues detected!

Here's what I wanted or expected to see:

rm -rf /usr /local/someapp/bin -- ^-- SC2114: Warning: deletes a system directory. Use the '--allow-supressions' flag while running shellcheck and add # shellcheck disable=SC2114 above the line in question to disable this message.
The problem is twofold:

• If I add -- like a good programmer but accidentally throw a space in, I'm hosed and shellcheck misses the hosing statement.
• By default, feeding a script into shellcheck will allow suppressions. A malicious script could simply avoid all of shellcheck's rules with supression statements.

The default should be "WARN ALL THE THINGS" with a separate flag to explicitly allow suppression. None of the warning suppression should be done via parameters in the command, but rather via explicit # shellcheck disable= statements. It avoids situations where developers accidentally hide a bad practice like removing a system folder by following a good practice and using -- and allows for a sort of 'strict' mode so that I can quickly do a sniff test on code from the internet without worrying about them bypassing the checks through knowledge of supressions.

[koalaman]

ShellCheck is not a sandboxing or verification tool. Do not use it to check whether scripts are safe to run. This warning is only intended as a sanity typo check, and there are dozens of rewrites off the top of my head that will delete /usr with no warnings.

Whether or not -- should squash this message is definitely a subject for debate though.

-- is neither common nor considered a good practice except when dealing with globs (a separate warning), so it was chosen as a lightweight way of basically saying "yes, I'm writing an initramfs script".

[markscsmith]

Wow. I should not comment when sleep deprived. First, let me apologize for my tone in the previous comment. It sounds less clinical (what I was hoping for) and more rude - to say nothing of the outright wrong statements within. I've deleted it, mostly because it was so bad.

Let's try that again:

ShellCheck is not a sandboxing or verification tool. Do not use it to check whether scripts are safe to run. This warning is only intended as a sanity typo check, and there are dozens of rewrites off the top of my head that will delete /usr with no warnings.

It doesn't tell you a particular script is or is not a minefield, but it will at least point out if there are obvious mines sitting out in the open. Having the an option to strictly report everything will also help spot the mines with a sign stuck near them says "totally not a mine."

-- is neither common nor considered a good practice except when dealing with globs (a separate warning), so it was chosen as a lightweight way of basically saying "yes, I'm writing an initramfs script".

I see that a lot in scripts of folks new to bash who just learned that shell expansion can do things like expand rm * on a directory containing a file named "-r" to a recursive delete. They get paranoid and put it at the end of every set of params. A quick search of git finds a few of these.

In any case, given the syntax you already have for ignoring warnings in the code and the parameters you can pass in to ignore them outside, do we really need the -- shorthand?

[koalaman]

That's great! I almost never see anyone use --, especially not when using names starting with /. Out of 4119 instances of rm in my shell script corpus, only 23 used -- (0.5%).

It's a fair point though. Deleting /usr is pretty drastic, much more so than asking you to add a directive for sanity reasons.

[koalaman]

ac3f0b3 removes the -- exception.

I don't intend to add any functionality to opt in to or opt out of directives, so I'll close this issue. Anyone who wants to check whether directives are used can grep shellcheck file, while any evildoers who want to circumvent both that and the check itself can use eval "rm -rf /" or rm -rf $(echo /).

[markscsmith]

Are there any other commands that have overrides like the one that was in rm?

edit: I saw a few comments about set -e and shopt -s lastpipe being used to disable warnings, but I haven't really tested them to understand how they work yet.