Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci(lint): Add differential-shellcheck action #360

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jamacku
Copy link

@jamacku jamacku commented Sep 11, 2022

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.

It performs differential ShellCheck scans and report results directly in pull request.

documentation: https://github.com/redhat-plumbers-in-action/differential-shellcheck

Signed-off-by: Jan Macku <[email protected]>
@abitrolly
Copy link
Collaborator

Why not full scan?

@jamacku
Copy link
Author

jamacku commented Sep 11, 2022

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.

@abitrolly
Copy link
Collaborator

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.

@jamacku
Copy link
Author

jamacku commented Sep 11, 2022

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.

@abitrolly
Copy link
Collaborator

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.

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