Add a security scan and use /validate and /security comment triggers#27
Open
Add a security scan and use /validate and /security comment triggers#27
Conversation
Suggestions on steps from Claude Code (earning credit line) Co-authored-by: Claude <claude@anthropic.com> Signed-off-by: Geoff Hutchison <geoff.hutchison@gmail.com>
Signed-off-by: Geoff Hutchison <geoff.hutchison@gmail.com>
Limit permissions to particular steps Be careful about manual workflow-dispatch names Signed-off-by: Geoff Hutchison <geoff.hutchison@gmail.com>
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive security scanning infrastructure for plugin validation. It adds a new security rules configuration file, creates a new security-scan GitHub Actions workflow with multi-phase scanning (detect changes, scan plugins in parallel, and post results), updates existing workflows to support dynamic plugin validation via CLI parameters and issue comments, and introduces new Python helper scripts for dependency extraction and plugin repository updates. Changes
Sequence Diagram(s)sequenceDiagram
actor PR as Pull Request
participant detect as detect-changes Job
participant parse as scripts/parse_plugins.py
participant scan as scan-plugin Job (Parallel)
participant bandit as Bandit/Semgrep/pip-audit
participant combine as Combine reports
participant post as post-results Job
participant gh as GitHub API
PR->>detect: Trigger on repositories.toml change
activate detect
detect->>parse: Run diff to find modified plugins
activate parse
parse-->>detect: Return plugin matrix JSON
deactivate parse
detect-->>scan: Output plugin list for parallel matrix
deactivate detect
par Parallel Scan Per Plugin
activate scan
scan->>bandit: Run Bandit, Semgrep, pip-audit
activate bandit
bandit-->>scan: Generate bandit-report.md, patterns-report.md, deps-report.md
deactivate bandit
scan->>combine: Collect per-plugin reports
end
deactivate scan
activate combine
combine->>combine: Concatenate reports → full-report.md
combine->>combine: Calculate overall risk level
combine-->>post: Upload artifacts per plugin
deactivate combine
activate post
post->>gh: Fetch/search for existing comment
alt Existing comment found
gh-->>post: Return comment ID
post->>gh: PATCH comment with updated report
else New comment
post->>gh: POST new PR comment
end
gh-->>post: Comment created/updated
deactivate post
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/avogadro-security.yaml:
- Around line 28-42: The avogadro-unsafe-deserialization rule currently flags
any yaml.load(...) call; update the rule (id: avogadro-unsafe-deserialization)
to exclude known safe-loader usages by adding pattern-not entries under
pattern-either for yaml.load(..., Loader=yaml.SafeLoader) and yaml.load(...,
Loader=SafeLoader) (or equivalent import alias), so the rule still matches
unsafe yaml.load(...) but does not fire when the Loader is explicitly set to
SafeLoader; keep the other unsafe patterns unchanged.
In @.github/workflows/security-scan.yml:
- Around line 562-575: The workflow currently only sets bandit_fail from
steps.bandit.outcome, so medium-only Bandit results fall through to LOW; add a
Bandit max-severity output (e.g. set bandit_max="${{
steps.bandit.outputs.max_severity }}" or similar) and include a check for
bandit_max == "MEDIUM" in the MEDIUM branch (or add bandit_max to the existing
elif alongside vuln_count, semgrep_max, semgrep_error, and audit_error) while
keeping the existing HIGH check that uses bandit_fail (steps.bandit.outcome)
unchanged.
- Around line 95-113: When building scan_list in the added/modified loops, skip
plugins that lack git information by checking the repo/commit fields (e.g.,
ensure p.get('repo') and the commit value are truthy) before appending; for
modified entries, validate p.get('new_commit') is non-empty as well. Update the
loops that iterate "for p in d['added']" and "for p in d['modified']" (and the
keys 'repo', 'commit', 'new_commit', and 'plugin_type') to guard and continue
when repo or commit is empty so src-based/non-git plugins are excluded from the
scan matrix.
- Around line 420-421: The extract-deps invocation (python3
_this/scripts/security_scan_helpers.py extract-deps "$src"
extracted-requirements.txt > deps-summary.json) can exit non-zero and be skipped
by the workflow because the step is marked continue-on-error; change the step so
failures are propagated by either removing continue-on-error or by explicitly
checking the command's exit status and, on non-zero, write the expected failure
artifacts (audit_error and vuln_count) and exit non-zero so the combine phase
sees the scan as failed; ensure you reference the extract-deps command, the
deps-summary.json output, and the audit_error/vuln_count sentinel files when
implementing the fix.
In @.github/workflows/validate-pr.yml:
- Around line 203-214: The fallback branch that posts the first PR comment uses
the command "gh pr comment ${{ steps.pr.outputs.number }} --body-file
comment.md" which can fail because the workspace isn't a git repo; update that
invocation to include the repository explicitly by adding "--repo ${{
github.repository }}" so it becomes "gh pr comment --repo ${{ github.repository
}} ${{ steps.pr.outputs.number }} --body-file comment.md" (leave the existing
PATCH path that updates an existing comment unchanged); reference the variables
comment_id and the gh pr comment command when making the change.
In `@scripts/security_scan_helpers.py`:
- Around line 43-46: The code currently emits raw Poetry-style constraints into
pip requirements (in the block that constructs requirement from spec and calls
_is_non_pypi); instead, detect and normalize Poetry version operators before
writing: in the function handling spec (the branch using variables spec and name
and helper _is_non_pypi) validate spec against PEP 440/PEP 508 forms (e.g.,
using packaging.version/packaging.specifiers or a small parser) and convert
common Poetry operators (caret ^ and tilde ~) into equivalent PEP 440
specifiers, and only return requirement when normalized; for any spec you cannot
normalize, do not produce a pip requirement—return a marker reason like
"manual-review:unsupported-poetry-constraint" so it surfaces for manual review
(apply same change to the other block referenced around lines 64-69).
- Around line 225-236: The loop over data.get("dependency-groups", {}) should
skip non-string entries before calling _is_non_pypi (which uses
DIRECT_URL_RE.search) to avoid TypeError on dict/table entries; update the inner
loop in the block that iterates group_name, deps to check isinstance(dep, str)
and if not, either skip (and optionally log) the entry instead of passing it to
_is_non_pypi, ensuring non_pypi and _append_requirement are only invoked for
string dependency specs (reference symbols: dependency-groups, dep,
_is_non_pypi, DIRECT_URL_RE.search, _append_requirement, non_pypi, pyproject).
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 989d6184-9491-4739-a586-4087bf917a98
📒 Files selected for processing (7)
.github/avogadro-security.yaml.github/workflows/check-plugin-updates.yml.github/workflows/security-scan.yml.github/workflows/validate-pr.ymlscripts/parse_plugins.pyscripts/security_scan_helpers.pyscripts/update_plugin_entry.py
Comment on lines
+95
to
+113
| for p in d['added']: | ||
| scan_list.append({ | ||
| 'name': p['name'], | ||
| 'repo': p['repo'], | ||
| 'commit': p['commit'], | ||
| 'path': p.get('path', '.'), | ||
| 'plugin_type': p.get('plugin_type', 'pypkg'), | ||
| 'change_type': 'added' | ||
| }) | ||
| for p in d['modified']: | ||
| if p.get('new_commit'): | ||
| scan_list.append({ | ||
| 'name': p['name'], | ||
| 'repo': p['repo'], | ||
| 'commit': p['new_commit'], | ||
| 'path': p.get('path', '.'), | ||
| 'plugin_type': p.get('plugin_type', 'pypkg'), | ||
| 'change_type': 'modified' | ||
| }) |
There was a problem hiding this comment.
Filter out non-git plugins before building the scan matrix.
cmd_diff() emits empty repo/commit values when a plugin uses src instead of git (scripts/parse_plugins.py does that for added and modified entries). Added plugins are appended here unconditionally, so a valid src-based plugin PR will fall over at git clone "" in the next job.
Minimal guard
for p in d['added']:
- scan_list.append({
- 'name': p['name'],
- 'repo': p['repo'],
- 'commit': p['commit'],
- 'path': p.get('path', '.'),
- 'plugin_type': p.get('plugin_type', 'pypkg'),
- 'change_type': 'added'
- })
+ if p.get('repo') and p.get('commit'):
+ scan_list.append({
+ 'name': p['name'],
+ 'repo': p['repo'],
+ 'commit': p['commit'],
+ 'path': p.get('path', '.'),
+ 'plugin_type': p.get('plugin_type', 'pypkg'),
+ 'change_type': 'added'
+ })
for p in d['modified']:
- if p.get('new_commit'):
+ if p.get('repo') and p.get('new_commit'):
scan_list.append({
'name': p['name'],
'repo': p['repo'],
'commit': p['new_commit'],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/security-scan.yml around lines 95 - 113, When building
scan_list in the added/modified loops, skip plugins that lack git information by
checking the repo/commit fields (e.g., ensure p.get('repo') and the commit value
are truthy) before appending; for modified entries, validate p.get('new_commit')
is non-empty as well. Update the loops that iterate "for p in d['added']" and
"for p in d['modified']" (and the keys 'repo', 'commit', 'new_commit', and
'plugin_type') to guard and continue when repo or commit is empty so
src-based/non-git plugins are excluded from the scan matrix.
Comment on lines
+420
to
+421
| python3 _this/scripts/security_scan_helpers.py extract-deps "$src" extracted-requirements.txt > deps-summary.json | ||
|
|
There was a problem hiding this comment.
Treat dependency-extraction failures as scan failures.
If extract-deps exits non-zero, this step stops before audit_error or vuln_count are written. Because the step is continue-on-error, the combine phase can still rate the plugin LOW even though dependency auditing never ran.
Minimal failure propagation
- python3 _this/scripts/security_scan_helpers.py extract-deps "$src" extracted-requirements.txt > deps-summary.json
+ set +e
+ python3 _this/scripts/security_scan_helpers.py extract-deps "$src" extracted-requirements.txt > deps-summary.json
+ extract_status=$?
+ set -e
+ if [ "$extract_status" -ne 0 ]; then
+ echo "⚠️ Dependency extraction failed." >> deps-report.md
+ echo "vuln_count=0" >> "$GITHUB_OUTPUT"
+ echo "audit_error=true" >> "$GITHUB_OUTPUT"
+ exit 0
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| python3 _this/scripts/security_scan_helpers.py extract-deps "$src" extracted-requirements.txt > deps-summary.json | |
| set +e | |
| python3 _this/scripts/security_scan_helpers.py extract-deps "$src" extracted-requirements.txt > deps-summary.json | |
| extract_status=$? | |
| set -e | |
| if [ "$extract_status" -ne 0 ]; then | |
| echo "⚠️ Dependency extraction failed." >> deps-report.md | |
| echo "vuln_count=0" >> "$GITHUB_OUTPUT" | |
| echo "audit_error=true" >> "$GITHUB_OUTPUT" | |
| exit 0 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/security-scan.yml around lines 420 - 421, The extract-deps
invocation (python3 _this/scripts/security_scan_helpers.py extract-deps "$src"
extracted-requirements.txt > deps-summary.json) can exit non-zero and be skipped
by the workflow because the step is marked continue-on-error; change the step so
failures are propagated by either removing continue-on-error or by explicitly
checking the command's exit status and, on non-zero, write the expected failure
artifacts (audit_error and vuln_count) and exit non-zero so the combine phase
sees the scan as failed; ensure you reference the extract-deps command, the
deps-summary.json output, and the audit_error/vuln_count sentinel files when
implementing the fix.
Comment on lines
+562
to
+575
| # Determine overall risk level | ||
| bandit_fail="${{ steps.bandit.outcome == 'failure' && 'true' || 'false' }}" | ||
| critical="${{ steps.patterns.outputs.critical }}" | ||
| vuln_count="${{ steps.deps.outputs.vuln_count }}" | ||
| semgrep_max="${{ steps.semgrep.outputs.max_severity }}" | ||
| semgrep_error="${{ steps.semgrep.outputs.tool_error }}" | ||
| audit_error="${{ steps.deps.outputs.audit_error }}" | ||
|
|
||
| if [ "$critical" = "true" ] || [ "$bandit_fail" = "true" ] || [ "$semgrep_max" = "ERROR" ]; then | ||
| echo "risk=HIGH" >> "$GITHUB_OUTPUT" | ||
| elif [ "${vuln_count:-0}" -gt 0 ] || [ "${{ steps.patterns.outputs.review }}" = "true" ] || [ "$semgrep_max" = "WARNING" ] || [ "$semgrep_error" = "true" ] || [ "$audit_error" = "true" ]; then | ||
| echo "risk=MEDIUM" >> "$GITHUB_OUTPUT" | ||
| else | ||
| echo "risk=LOW" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
Medium Bandit findings currently collapse to LOW risk.
steps.bandit.outcome == 'failure' only covers HIGH findings and tool errors in this workflow. A plugin with only medium Bandit results falls through to LOW unless another scanner happens to trip, even though the Bandit report already classified those findings as noteworthy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/security-scan.yml around lines 562 - 575, The workflow
currently only sets bandit_fail from steps.bandit.outcome, so medium-only Bandit
results fall through to LOW; add a Bandit max-severity output (e.g. set
bandit_max="${{ steps.bandit.outputs.max_severity }}" or similar) and include a
check for bandit_max == "MEDIUM" in the MEDIUM branch (or add bandit_max to the
existing elif alongside vuln_count, semgrep_max, semgrep_error, and audit_error)
while keeping the existing HIGH check that uses bandit_fail
(steps.bandit.outcome) unchanged.
Comment on lines
+43
to
+46
| if isinstance(spec, str): | ||
| requirement = name if spec in {"", "*"} else f"{name}{spec}" | ||
| reason = "direct URL dependency" if _is_non_pypi(spec) else None | ||
| return requirement, reason |
There was a problem hiding this comment.
Don't emit raw Poetry constraints as pip requirements.
tool.poetry version strings are not guaranteed to be valid PEP 508 fragments. Values like "1.2.3" or "^1.2" become invalid lines here (pkg1.2.3, pkg^1.2), which will make pip-audit -r fail on otherwise valid Poetry projects. Please normalize Poetry syntax first, or surface unsupported forms as manual-review items instead of writing them into requirements.txt.
Also applies to: 64-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/security_scan_helpers.py` around lines 43 - 46, The code currently
emits raw Poetry-style constraints into pip requirements (in the block that
constructs requirement from spec and calls _is_non_pypi); instead, detect and
normalize Poetry version operators before writing: in the function handling spec
(the branch using variables spec and name and helper _is_non_pypi) validate spec
against PEP 440/PEP 508 forms (e.g., using
packaging.version/packaging.specifiers or a small parser) and convert common
Poetry operators (caret ^ and tilde ~) into equivalent PEP 440 specifiers, and
only return requirement when normalized; for any spec you cannot normalize, do
not produce a pip requirement—return a marker reason like
"manual-review:unsupported-poetry-constraint" so it surfaces for manual review
(apply same change to the other block referenced around lines 64-69).
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
matterhorn103
approved these changes
Mar 29, 2026
Collaborator
matterhorn103
left a comment
matterhorn103
left a comment
There was a problem hiding this comment.
I wouldn't say I know enough about this kind of thing to be all that helpful but looks good from what I can tell :)
Collaborator
|
Something that might help us with reviewing updates is if any PR that updates a plugin would have a git diff file generated and uploaded, containing the diff between the proposed commit and the one currently in the index/repo. |
Member
Author
|
I'll take this up later in the week. Having a |
Collaborator
|
Yeah, I'd suggest only running it for updates to existing plugins, not completely new additions. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Suggestions on steps from Claude Code (earning credit line)
Summary by CodeRabbit
Release Notes
New Features
/validateand/securityslash commands in issue comments.Chores