cheat.sh icon indicating copy to clipboard operation
cheat.sh copied to clipboard

ci(lint): Add differential-shellcheck action

Open jamacku opened this issue 3 years ago • 5 comments

Differential ShellCheck is a GitHub action that performs differential ShellCheck scans on shell scripts changed via PR and reports results directly in PR. The beauty of differential scans is that you will get reports only about newly added defects.

Since this repository has few shell scripts, I think you might find some value in having a shell linter. Differential ShellCheck action is able to produce reports in SARIF format. GitHub understands this format and is able to display it nicely as a PR comment, and on the Files Changed tab, please see below.

image

image

Documentation is available at @redhat-plumbers-in-action/differential-shellcheck. Let me know If you are missing some feature or setting. I'm always happy to extend functionality.

jamacku avatar Sep 11 '22 14:09 jamacku

Why not full scan?

abitrolly avatar Sep 11 '22 15:09 abitrolly

Why not full scan?

Doing a full scan on most repositories usually produce a lot of noise ("old defects"). Therefore, it's challenging for projects with a lot of shell scripts to introduce ShellCheck linter. Differential scans help to focus only on changes introduced in PR. At the same time, you can work on fixing/masking old defects.

You can have a look at your current "defects" by executing:

shellcheck \
  --external-sources \
  --severity=warning \
  $( \
    grep -r -E '(^\s*((\#|\!)|(\#\s*\!)|(\!\s*\#))\s*(\/usr(\/local)?)?\/bin\/(env\s+)?(sh|ash|bash|dash|ksh|bats)\b)|(^\s*\#\s+-\*-\s+(sh|ash|bash|dash|ksh|bats)\s+-\*-\s*)|(^\s*\#\s*shellcheck\s+shell=(sh|ash|bash|dash|ksh|bats)\s*)' './' \
    | cut -d: -f1 \
  )

Currently, there are 19 defects with a severity warning or higher. When including also note severity, it is 195 defects. Not all defects are "bugs"; therefore, it's helpful to have differential scans for PRs.

jamacku avatar Sep 11 '22 16:09 jamacku

I don't think it catches https://github.com/chubin/cheat.sh/blob/master/share/cht.sh.txt and while it uses eval (which I don't understand enough to replace it) I don't believe these spellcheck rules would make the code more secure.

abitrolly avatar Sep 11 '22 16:09 abitrolly

It should also catch changes in cht.sh.txt since shebang is defined.

ShellCheck doesn't necessarily make your code base more secure, but it can help you to review PRs and potentially help you catch some bugs during the review process.

jamacku avatar Sep 11 '22 16:09 jamacku

I don't mind against CI/CD checks, but it is hard to dedicate time for reviewing polished bash script while the major security problem (getting rid of eval) stands unsolved.

abitrolly avatar Sep 11 '22 16:09 abitrolly