Sync with upstream and update README and dependencies - #18
Sync with upstream and update README and dependencies#18reactive-firewall wants to merge 7 commits into
Conversation
* main: re:re:re updated the usage example in the README.md Update README.md Update README.md Update name in action.yml [UPDATE] (deps): Bump github/codeql-action from 2 to 3 Update README.md Allow config file specification Upgrade upload-serif action to v3 Fix breaking changes in upload-artifact action use
…t-scan for use by shundor/python-bandit-scan
* Oops! :hear_not_evil: The labels must already be present in the github project settings. (FIXED) * I also don't have write-access so I removed myself from the default assignee; this simplifies the configuration. These are not user-facing changes.
# Patch Notes Just bringing dev up-to-date with recent version bumps. ## Impacted GHIs: * None ## Included & Superseded PR/MRs * Includes #5 * Includes & Supersedes #6 * Includes & Supersedes #7 * Includes & Supersedes #11 * Includes & Supersedes #12 * Includes & Supersedes #13 * Includes & Supersedes #14 ---
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Mr. Walls <reactive-firewall@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Abir Majumdar <83433840+abirismyname@users.noreply.github.com>
reactive-firewall
added
documentation
📝 WalkthroughWalkthroughThe action now passes Bandit configuration arguments separately and clears inherited configuration for the default case. The README clarifies that ChangesBandit behavior updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@action.yml`:
- Around line 117-123: Update the README configuration-input documentation to
add the config_path option alongside ini_path, describing that it passes a
YAML/TOML Bandit configuration file via -c. Keep the existing ini_path
documentation unchanged and accurately reflect the action.yml interface.
- Around line 119-123: Update the Bandit invocation around CONFIG_PATH and
CONFIG_PATH_ARG to store the optional config flag and path as a Bash array,
preserving INPUT_CONFIG_PATH as one argument even when it contains spaces or
wildcard characters. Expand that array with quoted "${CONFIG_PATH_ARGS[@]}" in
the bandit command, while keeping the existing behavior when no config path is
provided.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cabb41b0-a79a-4994-bf5d-7fcfa6e052e9
📒 Files selected for processing (2)
README.mdaction.yml
| if [ -n "$INPUT_CONFIG_PATH" ] ; then | ||
| # Set both (but let bandit validate its own arguments) | ||
| CONFIG_PATH="$INPUT_CONFIG_PATH" # quote paths on assignment | ||
| CONFIG_PATH_ARG="-c" | ||
| fi | ||
| fi | ||
| bandit -f sarif -o results.sarif -r $INPUT_PATH $LEVEL $CONFIDENCE $EXCLUDED_PATHS $EXIT_ZERO $SKIPS $INI_PATH $CONFIG_PATH | ||
| bandit -f sarif -o results.sarif -r $INPUT_PATH $LEVEL $CONFIDENCE $EXCLUDED_PATHS $EXIT_ZERO $SKIPS $INI_PATH $CONFIG_PATH_ARG ${CONFIG_PATH:-} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 2 'config_path|ini_path|INPUT_CONFIG_PATH' action.yml README.mdRepository: reactive-firewall/python-bandit-scan
Length of output: 1768
Add config_path to the README.
action.yml defines and passes config_path to Bandit with -c, including the YAML/TOML support. The README currently documents only ini_path for .bandit input. Add a config_path entry so users know both configuration mechanisms are available.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@action.yml` around lines 117 - 123, Update the README configuration-input
documentation to add the config_path option alongside ini_path, describing that
it passes a YAML/TOML Bandit configuration file via -c. Keep the existing
ini_path documentation unchanged and accurately reflect the action.yml
interface.
| CONFIG_PATH="$INPUT_CONFIG_PATH" # quote paths on assignment | ||
| CONFIG_PATH_ARG="-c" | ||
| fi | ||
| fi | ||
| bandit -f sarif -o results.sarif -r $INPUT_PATH $LEVEL $CONFIDENCE $EXCLUDED_PATHS $EXIT_ZERO $SKIPS $INI_PATH $CONFIG_PATH | ||
| bandit -f sarif -o results.sarif -r $INPUT_PATH $LEVEL $CONFIDENCE $EXCLUDED_PATHS $EXIT_ZERO $SKIPS $INI_PATH $CONFIG_PATH_ARG ${CONFIG_PATH:-} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
CONFIG_PATH='configs/security policy.yaml'
CONFIG_PATH_ARG='-c'
printf '<%s>\n' $CONFIG_PATH_ARG ${CONFIG_PATH:-}Repository: reactive-firewall/python-bandit-scan
Length of output: 212
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'Files:\n'
git ls-files | rg '(^|/)action\.y(a)?ml$|(^|/)README\.md$' || true
printf '\nRelated action.yml section:\n'
sed -n '90,145p' action.yml
printf '\nSearch config options:\n'
rg -n "CONFIG_PATH|input_config_path|config:" README.md action.yml || trueRepository: reactive-firewall/python-bandit-scan
Length of output: 2743
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'Demonstrating Bash expansion behavior:\n'
for VALUE in 'configs/security policy.yaml' 'configs/*.yaml' ''; do
CONFIG_PATH_ARG='-c'
CONFIG_PATH="$VALUE"
printf 'Input: <%s>\n' "$VALUE"
printf 'printf outputs:\n'
printf '<%s>\n' $CONFIG_PATH_ARG ${CONFIG_PATH:-} || true
printf '\n'
done
printf 'Demonstrating Bash array behavior:\n'
CONFIG_PATH_ARGS=(-c 'configs/security policy.yaml')
printf 'printf outputs:\n'
printf '<%s>\n' "${CONFIG_PATH_ARGS[@]}"
printf '\n'Repository: reactive-firewall/python-bandit-scan
Length of output: 489
Preserve INPUT_CONFIG_PATH as one Bandit argument.
Line 119 quotes the assignment, but line 123 expands ${CONFIG_PATH:-} without quotes. Bash will split whitespace- or wildcard-containing path values into multiple arguments. A path such as configs/security policy.yaml then sends configs/security and policy.yaml to Bandit, and an extra token can be mistaken for a Bandit option. Use a Bash array for the optional option and path, and expand it with "${CONFIG_PATH_ARGS[@]}".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@action.yml` around lines 119 - 123, Update the Bandit invocation around
CONFIG_PATH and CONFIG_PATH_ARG to store the optional config flag and path as a
Bash array, preserving INPUT_CONFIG_PATH as one argument even when it contains
spaces or wildcard characters. Expand that array with quoted
"${CONFIG_PATH_ARGS[@]}" in the bandit command, while keeping the existing
behavior when no config path is provided.
There was a problem hiding this comment.
🙅🏻 While I appreciate the idea, your suggestion seems to introduce abstractions that may violate the Avoid Hasty Abstraction principle. Namely you assume it is desirable to use a bash array, and that bandit will handle bandit -f sarif -o results.serif -r . -ll -i "" (defaults with your suggestion) which will introduce calls where "" (e.g., quoted empty string) is passed after arguments that do not take meta-vars during the bandit invocation.
This causes a regression. The current logic ensures -c when passed a file path that the file path has been re-quoted already so AFAIK bash will see the ${...} pattern and treat the value as a single token (e.g. "example with spaces" -> example\ with\ spaces) albeit a little fragile. Does that make sense?
There was a problem hiding this comment.
@reactive-firewall, the concern about an empty argument is valid to check, but it does not occur with the proposed array form.
If CONFIG_PATH_ARGS=(), then:
"${CONFIG_PATH_ARGS[@]}"expands to zero arguments. It does not pass "" to bandit. The command therefore retains the current no-config-path behavior.
The current ${CONFIG_PATH:-} expansion remains unsafe. Quotes in the value do not become shell quotes during a later expansion. Bash treats them as ordinary characters. It then performs word splitting and pathname expansion because the parameter expansion is unquoted.
For example, if the input is configs/security policy.yaml, this:
${CONFIG_PATH:-}becomes two arguments. It does not become one escaped argument.
The array is not an additional abstraction for its own sake. It models the required command arguments: either no config arguments, or exactly -c plus one path argument. That preserves the argument boundary without passing an empty argument.
The original finding remains applicable.
🐇⬛
You are interacting with an AI system.
There was a problem hiding this comment.
Ok, I see where you're going with this, but let's prove it with test code before making any changes.
Test Requirements:
- demonstrate how passing arguments actual expands (both on the bash side and the bandit (python) side)
- verify that the expected value would reach the bandit (python) side exactly (use monkey patching on bandit's CLI code if you have to)
- hermetic test
- proves your suggestion
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' 'Repository files relevant to tests and tooling:'
fd -HI -t f \
-e yml -e yaml -e py -e sh -e toml -e ini -e cfg \
-g '!*node_modules*' -g '!*dist*' -g '!*build*' \
| sort
printf '%s\n' 'Root-level metadata:'
fd -HI -t f -d 1 | sort
printf '%s\n' 'Test and Bandit references:'
rg -n -C 3 \
-e 'pytest|unittest|bats|shellcheck|bandit|action\.yml|composite' \
-g '!*node_modules*' -g '!*dist*' -g '!*build*' \
. || trueLength of output: 990
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' 'Repository files relevant to tests and tooling:'
fd -HI -t f . \
--exclude node_modules --exclude dist --exclude build \
-e yml -e yaml -e py -e sh -e toml -e ini -e cfg \
| sort
printf '%s\n' 'Root-level metadata:'
fd -HI -t f -d 1 . | sort
printf '%s\n' 'Test and Bandit references:'
rg -n -C 3 \
-e 'pytest|unittest|bats|shellcheck|bandit|action\.yml|composite' \
-g '!node_modules/**' -g '!dist/**' -g '!build/**' \
. || trueLength of output: 4264
@reactive-firewall, the repository has no existing test framework or test workflow. I recommend one self-contained Bash regression test and one small workflow to run it.
The test will execute the actual Run Bandit scan script extracted from action.yml. It will not copy the config-path logic into the test.
The test will place a local bandit shim first in PATH:
- The Bash shim records its received
"$@"argument vector. - The shim starts a local Python recorder.
- The Python recorder records
sys.argv. - The test compares both records with the expected argument vector.
This proves the Bash boundary and the Python CLI boundary without installing Bandit or accessing the network.
- Add a hermetic test that executes the current action command through local Bash and Python shims.
- Cover a config path containing whitespace and a literal wildcard.
- Cover the
DEFAULTconfig-path case, which must pass no-coption and no empty argument. - Update
action.ymlonly after the test shows the current split-path failure. - Add a minimal workflow that runs the test on pull requests.
- 📌 Create a pull request with these changes
You are interacting with an AI system.
2 participants
Patch Notes
Resolves merge conflicts and synchronizes with upstream to update README and related changes from review.
Impacted GHIs:
Included & Superseded PR/MRs
shundor/python-bandit-scanshundor/python-bandit-scan#6Summary by CodeRabbit
Bug Fixes
Documentation
exit_zerobehavior is determined by scan results.