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

Add an option to check all executable scripts #109

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions .github/workflows/scandir.yml
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,19 @@ jobs:
echo "::error:: Expected file $notexpect found in ${{ steps.two.outputs.files }}"
exit 1
fi

- name: Run ShellCheck
uses: ./
id: three
with:
scandir: './testfiles/scandir'
all_scripts: true

- name: Verify check
run: |
expect="testfiles/scandir/unknown-shebang"

if [[ ! "${{ steps.two.outputs.files }}" =~ $expect ]];then
echo "::error:: Expected file $expect not found in ${{ steps.two.outputs.files }}"
exit 1
fi
mislav marked this conversation as resolved.
Show resolved Hide resolved
mislav marked this conversation as resolved.
Show resolved Hide resolved
8 changes: 7 additions & 1 deletion action.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ inputs:
description: "A space separated list of additional filename to check"
required: false
default: ""
all_scripts:
description: "Set to true to check all executable scripts. The default is to check only ones with known shebangs"
required: false
default: ""
ignore:
description: "Paths to ignore when running ShellCheck"
required: false
Expand Down Expand Up @@ -147,6 +151,7 @@ runs:
shell: bash
id: check
env:
INPUT_ALL_SCRIPTS: ${{ inputs.all_scripts }}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Implementation looks good, but consider documenting security implications.

The technical implementation correctly implements the new functionality to check all executable scripts with any shebang when all_scripts is true. However, there are some important considerations:

  1. Security: Checking all executable files with any shebang could potentially process non-shell scripts, which might lead to false positives or waste processing time.
  2. Performance: This change could significantly increase the number of files being processed in large repositories.

Consider adding documentation in the README to:

  • Explain the security implications of enabling all_scripts
  • Provide guidance on when to use this option vs. additional_files
  • Include examples of scenarios where this option is beneficial (e.g., HomeAssistant addons)

Also applies to: 163-164

INPUT_SCANDIR: ${{ inputs.scandir }}
INPUT_CHECK_TOGETHER: ${{ inputs.check_together }}
INPUT_EXCLUDE_ARGS: ${{ steps.exclude.outputs.excludes }}
Expand All @@ -155,7 +160,8 @@ runs:
run: |
statuscode=0
declare -a filepaths
shebangregex="^#! */[^ ]*/(env *)?[abk]*sh"
shebangregex="^#!"
[ "$INPUT_ALL_SCRIPTS" = "true" ] || shebangregex="^#! */[^ ]*/(env *)?[abk]*sh"

set -f # temporarily disable globbing so that globs in inputs aren't expanded

Expand Down
3 changes: 3 additions & 0 deletions testfiles/scandir/unknown-shebang
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
#!/usr/bin/with-contenv bashio

echo "hi"
mislav marked this conversation as resolved.
Show resolved Hide resolved