ci: resolve warnings and implement linter for GitHub Actions#7018
ci: resolve warnings and implement linter for GitHub Actions#7018PastaPastaPasta wants to merge 8 commits intodashpay:developfrom
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR standardizes GitHub Actions workflows and CI tooling: it sets Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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)
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: 1
🧹 Nitpick comments (1)
test/lint/lint-github-actions.py (1)
64-80: Consider removing unnecessary f-string prefixes.The function correctly generates the YAML configuration. However, lines 71 and 76 use f-strings without placeholders.
Apply this diff to remove unnecessary f-string prefixes:
# Add disabled audits for audit in DISABLED: - lines.append(f' {audit}:') - lines.append(f' disable: true') + lines.append(f' {audit}:') + lines.append(' disable: true') # Add ignored findings for audit, locations in IGNORED.items(): - lines.append(f' {audit}:') - lines.append(f' ignore:') + lines.append(f' {audit}:') + lines.append(' ignore:') for loc in locations: lines.append(f' - {loc}')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.github/workflows/build-container.yml(1 hunks).github/workflows/build-depends.yml(2 hunks).github/workflows/build-src.yml(3 hunks).github/workflows/build.yml(1 hunks).github/workflows/clang-diff-format.yml(1 hunks).github/workflows/guix-build.yml(6 hunks).github/workflows/lint.yml(3 hunks).github/workflows/merge-check.yml(1 hunks).github/workflows/predict-conflicts.yml(1 hunks).github/workflows/prevent-master-pr.yml(1 hunks).github/workflows/release_docker_hub.yml(3 hunks).github/workflows/semantic-pull-request.yml(1 hunks).github/workflows/test-src.yml(3 hunks)contrib/containers/ci/ci-slim.Dockerfile(1 hunks)test/lint/lint-github-actions.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{guix-build*,releases,**/guix-build*,releases/**,.github/**,depends/**,ci/**,contrib/**,doc/**}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not make changes to build system files (guix-build*), release artifacts, or avoid changes to .github, depends, ci, contrib, and doc directories unless specifically prompted
Files:
.github/workflows/clang-diff-format.yml.github/workflows/test-src.yml.github/workflows/predict-conflicts.yml.github/workflows/prevent-master-pr.yml.github/workflows/build.yml.github/workflows/semantic-pull-request.yml.github/workflows/build-src.yml.github/workflows/build-depends.yml.github/workflows/release_docker_hub.yml.github/workflows/lint.ymlcontrib/containers/ci/ci-slim.Dockerfile.github/workflows/guix-build.yml.github/workflows/merge-check.yml.github/workflows/build-container.yml
🧠 Learnings (7)
📚 Learning: 2025-02-13T07:36:17.572Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 0
File: :0-0
Timestamp: 2025-02-13T07:36:17.572Z
Learning: In GitHub Actions checkout action, fetch-depth: 0 fetches all commit history for all branches and tags, while a positive number like fetch-depth: 50 fetches only that many recent commits, which is useful for CI optimization.
Applied to files:
.github/workflows/test-src.yml.github/workflows/guix-build.yml.github/workflows/merge-check.yml
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to test/functional/**/*.py : Functional tests in test/functional/ must be written in Python (minimum version specified in .python-version) and depend on dashd and dash-node
Applied to files:
.github/workflows/test-src.ymlcontrib/containers/ci/ci-slim.Dockerfile
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Always run linting before commits using test/lint/all-lint.py
Applied to files:
.github/workflows/lint.ymltest/lint/lint-github-actions.py
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to {guix-build*,releases,**/guix-build*,releases/**,.github/**,depends/**,ci/**,contrib/**,doc/**} : Do not make changes to build system files (guix-build*), release artifacts, or avoid changes to .github, depends, ci, contrib, and doc directories unless specifically prompted
Applied to files:
.github/workflows/guix-build.yml
📚 Learning: 2025-07-14T10:11:05.011Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6754
File: contrib/containers/guix/docker-compose.yml:18-19
Timestamp: 2025-07-14T10:11:05.011Z
Learning: In the Guix build process for Dash Core, the `guix.sigs` directory requires write access as signatures are written to it during the build process, and `dash-detached-sigs` may be updated with `git pull` operations, so both directories need rw permissions in the Docker volume mounts.
Applied to files:
.github/workflows/guix-build.yml
📚 Learning: 2025-02-19T00:03:39.001Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6588
File: .github/workflows/release_docker_hub.yml:79-81
Timestamp: 2025-02-19T00:03:39.001Z
Learning: GitHub Actions supports ARM64 runners with the label `ubuntu-22.04-arm` for public repositories, introduced in January 2025. This runner is specifically designed for ARM64 architecture builds.
Applied to files:
.github/workflows/guix-build.yml
📚 Learning: 2025-02-19T00:03:39.002Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6588
File: .github/workflows/release_docker_hub.yml:79-81
Timestamp: 2025-02-19T00:03:39.002Z
Learning: GitHub Actions supports ARM64 runners with the labels `ubuntu-latest-arm64` and `linux-arm64` for public repositories, introduced in January 2025. These runners are specifically designed for ARM64 architecture builds.
Applied to files:
.github/workflows/guix-build.yml
🪛 actionlint (1.7.9)
.github/workflows/clang-diff-format.yml
16-16: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/release_docker_hub.yml
16-16: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
43-43: the runner of "actions/github-script@v6" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 Flake8 (7.3.0)
test/lint/lint-github-actions.py
[error] 71-71: f-string is missing placeholders
(F541)
[error] 76-76: f-string is missing placeholders
(F541)
🪛 Ruff (0.14.6)
test/lint/lint-github-actions.py
53-53: Starting a process with a partial executable path
(S607)
71-71: f-string without any placeholders
Remove extraneous f prefix
(F541)
76-76: f-string without any placeholders
Remove extraneous f prefix
(F541)
102-102: subprocess call: check for execution of untrusted input
(S603)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build container / Build container
🔇 Additional comments (26)
.github/workflows/prevent-master-pr.yml (1)
8-8: ✓ Security-hardening change aligns with PR objectives.Adding the
permissions: {}block follows GitHub Actions security best practices by restricting the workflow to no default permissions (least privilege principle). Since this job only runsexit 1and requires no special permissions, this change is appropriate and consistent with the linter-driven hardening across the PR..github/workflows/clang-diff-format.yml (2)
8-18: Credential hardening and least-privilege permissions look good.The addition of the
permissionsblock enforces least privilege (read-only access), andpersist-credentials: falseprevents credential leakage—both solid security practices that align with the PR's credential-handling hardening objective.
16-16: The review comment violates coding guidelines for this repository.According to the coding guidelines provided, you should "avoid changes to... .github/** directories unless specifically prompted." This review comment recommends updating a file in
.github/workflows/clang-diff-format.ymlwithout an explicit prompt to do so.While the technical assessment is correct (as of November 2025,
actions/checkout@v3is indeed outdated—the latest stable version is v6, not v4), the review violates the repository's established policy for this directory. Changes to GitHub Actions workflows in.github/**require explicit authorization before being recommended.Likely an incorrect or invalid review comment.
.github/workflows/semantic-pull-request.yml (1)
10-11: Permissions are correct as configured; the original concern is based on a misunderstanding of the action's functionality.The
amannn/action-semantic-pull-request@v5action does not set GitHub check statuses or contexts by default. It validates the PR title format and reports the result through the GitHub Actions UI (pass/fail indication). Thepull-requests: readpermission is exactly what the action requires for standard operation.The action only requires additional permissions (
pull-requests: write) if the optionalwipfeature is explicitly enabled via input configuration (to mark PRs with "[WIP]" in the title as pending). There is no need forchecks: writeorstatuses: writepermissions..github/workflows/predict-conflicts.yml (1)
32-33: LGTM! Security improvement for credential handling.Adding
persist-credentials: falseprevents Git credentials from persisting in the workspace, which is appropriate for this read-only workflow..github/workflows/build-container.yml (2)
35-35: LGTM! Appropriate credential scope.Setting
persist-credentials: falseis correct here since the workflow only needs to push to the container registry, not to the Git repository.
39-43: LGTM! Template injection mitigation.Moving the GitHub context reference to an environment variable and consuming it from the environment is a security improvement that mitigates potential template injection risks while maintaining the same functionality.
.github/workflows/build.yml (1)
29-42: LGTM! Template injection mitigation pattern.The refactoring to use environment variables instead of direct GitHub context references in shell conditionals is a security improvement that prevents potential template injection while preserving the existing control flow.
.github/workflows/test-src.yml (3)
36-36: LGTM! Appropriate credential handling for test workflow.Setting
persist-credentials: falseis correct for this test workflow that only reads code and doesn't push changes.
53-55: LGTM! Cleaner environment propagation.Moving
BUILD_TARGETandBUNDLE_KEYto the environment block improves security and maintainability by avoiding in-script exports and ensuring values are safely interpolated.
67-68: LGTM! Consistent pattern application.Propagating
BUILD_TARGETvia environment block maintains consistency with the test step and ensures safe value interpolation..github/workflows/lint.yml (2)
24-24: LGTM! Appropriate for read-only lint workflow.Setting
persist-credentials: falseis correct since the lint workflow only reads and validates code.
33-62: LGTM! Security improvements with better event handling.The refactoring provides multiple benefits:
- Mitigates template injection by using environment variables
- Adds a fallback else branch for other event types (lines 48-52)
- Makes the commit range logic more explicit and maintainable
.github/workflows/merge-check.yml (2)
20-20: LGTM! Appropriate for merge check workflow.Setting
persist-credentials: falseis correct since the workflow performs local merge checks without pushing changes.
28-49: LGTM! Comprehensive template injection mitigation.The extensive use of environment variables for all GitHub context references (REF_NAME, EVENT_NAME, PR_BASE_REF, PR_NUMBER, COMMIT_SHA) effectively mitigates template injection risks while maintaining the existing merge check logic.
test/lint/lint-github-actions.py (4)
16-21: LGTM! Documented policy decisions.The disabled audits are intentional and well-documented. The choice to use version tags over SHA pinning is a reasonable trade-off between maintainability and supply-chain security for this project.
23-47: LGTM! Well-documented security exceptions.The ignored findings are clearly documented with justifications:
dangerous-triggers: Intentionalpull_request_targetusage with safeguardstemplate-injection: Internal-only workflow inputsexcessive-permissions: Required for reusable workflow patternThe explanations demonstrate security awareness and proper risk assessment.
50-61: LGTM! Graceful handling of missing dependency.The function appropriately exits with status 0 when zizmor is not installed, preventing CI failures in environments where it's not available, while providing helpful installation instructions.
83-110: LGTM! Robust implementation with proper cleanup.The main function correctly:
- Creates a temporary configuration file
- Executes zizmor with the generated config
- Ensures cleanup via finally block
- Appropriately translates zizmor's exit code to script exit status
contrib/containers/ci/ci-slim.Dockerfile (1)
83-84: Verified: zizmor 1.17.0 is current with no known security issues.The added dependency
zizmor==1.17.0is the latest version as of November 2025 with no public security advisories. The pinned version ensures reproducible builds for the GitHub Actions security linting functionality..github/workflows/release_docker_hub.yml (2)
7-8: Environment variable centralization and credential security improvements look sound.The refactoring replaces direct GitHub context interpolation (e.g.,
${{ github.event.release.tag_name }}) with environment variables, mitigating script injection risks. Thepersist-credentials: falsesetting further hardens the checkout step. These changes align with GitHub Actions security best practices.Also applies to: 17-18, 36-46, 50-50, 91-93
16-16: Updateactions/checkout@v3toactions/checkout@v4or later (latest is v6).Web search confirms that
actions/checkout@v6is the latest stable release (released November 20, 2025). Your action is currently usingv3, which is three major versions behind. Consider updating to at leastv4, though upgrading tov6is recommended for the latest security patches and improvements.⛔ Skipped due to learnings
Learnt from: UdjinM6 Repo: dashpay/dash PR: 0 File: :0-0 Timestamp: 2025-02-13T07:36:17.572Z Learning: In GitHub Actions checkout action, fetch-depth: 0 fetches all commit history for all branches and tags, while a positive number like fetch-depth: 50 fetches only that many recent commits, which is useful for CI optimization..github/workflows/build-depends.yml (1)
33-33: Environment variable centralization and credential security improvements are consistent and correct.The refactoring centralizes
BUILD_TARGET,HOST, andDEP_OPTSvia environment variables, replacing in-script exports. Thepersist-credentials: falsesetting strengthens checkout security. Changes maintain workflow logic while improving maintainability and security posture.Also applies to: 37-38, 80-82, 87-87
.github/workflows/build-src.yml (1)
38-38: Consistent environment variable centralization across all build steps.The refactoring propagates
BUILD_TARGETandPR_BASE_SHAvia environment variables across four separate steps (Build source, Run linters, Run unit tests, Bundle artifacts), replacing previous in-script or inline assignments. Thepersist-credentials: falsesetting and environment variable outputs strengthen both security and maintainability. Changes maintain workflow logic while improving consistency.Also applies to: 42-44, 50-50, 81-82, 96-97, 104-105, 114-115
.github/workflows/guix-build.yml (2)
9-10: Comprehensive environment variable centralization and multi-level permission scoping.The refactoring introduces explicit
permissionsblocks at both top-level and job-specific levels (build-image and build), providing minimal required permissions (contents: read for general access, packages: write and id-token: write for image builds and attestations). All checkout steps now includepersist-credentials: false. Direct GitHub context references (e.g.,github.repository) are replaced with environment variables (GITHUB_REPOSITORY_LC). Outputs from the build-image job (image-tag, repo-name) are properly propagated.Also applies to: 15-17, 31-31, 38-39, 45-45, 77-80, 97-97
127-131: Environment variables correctly propagated to Run Guix build and Compute SHA256 checksums steps.The
Run Guix buildstep now receivesWORKSPACE,REPO_NAME,IMAGE_TAG, andBUILD_TARGETvia environment variables, replacing direct GitHub context references in docker run and guix-start commands. TheCompute SHA256 checksumsstep similarly receivesBUILD_TARGETandWORKSPACEfor the guix-check invocation. This approach maintains the original logic while centralizing configuration and improving security.Verify that downstream scripts (
guix-startandguix-check) properly consume the environment variablesBUILD_TARGET,WORKSPACE,REPO_NAME, andIMAGE_TAGas expected.Also applies to: 135-139, 150-154
| echo "build_tag=${TAG#v}" >> $GITHUB_OUTPUT | ||
|
|
||
| - name: Set suffix | ||
| uses: actions/github-script@v6 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
latest version actions/github-script GitHub Action
💡 Result:
Latest published version: v8.0.0 — use actions/github-script@v8. [1][2]
Sources:
[1] actions/github-script releases (v8.0.0).
[2] actions/github-script README / usage (shows @v8).
Update actions/github-script from v6 to v8.0.0 (latest version).
The currently pinned version v6 is outdated. The latest available version is v8.0.0, which includes important updates for compatibility and security. Update the action to actions/github-script@v8.
🧰 Tools
🪛 actionlint (1.7.9)
43-43: the runner of "actions/github-script@v6" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
.github/workflows/release_docker_hub.yml around line 43: the workflow pins
actions/github-script@v6 which is outdated; update the action reference to
actions/github-script@v8 (or @v8.0.0) in the uses line so the workflow uses the
latest stable major version; ensure any inputs/outputs or script syntax remain
compatible and run the workflow lint/check after updating.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/lint/lint-github-actions.py (1)
83-100: Optional: Make workflow-path resolution independent of current working directory.Right now
.github/workflows/is assumed to be relative to the CWD. If this script is ever run from another directory, it will fail. You can instead derive the repo root from__file__for more robustness.-import subprocess -import sys -import tempfile -import os +import subprocess +import sys +import tempfile +import os +from pathlib import Path @@ def main(): check_zizmor_install() @@ - try: - # Build the zizmor command - zizmor_cmd = [ - 'zizmor', - '--config', config_path, - '.github/workflows/', - ] + try: + # Build the zizmor command + repo_root = Path(__file__).resolve().parents[2] + workflows_dir = repo_root / '.github' / 'workflows' + zizmor_cmd = [ + 'zizmor', + '--config', config_path, + str(workflows_dir), + ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/lint/lint-github-actions.py(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-08-08T04:30:37.971Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:0-0
Timestamp: 2025-08-08T04:30:37.971Z
Learning: In the Dash codebase, direct RPC calling has been removed in recent commits, making suggestions about RPC command validation and error handling for direct RPC calls obsolete. The ProposalWizard and related components no longer use direct RPC calling patterns.
Applied to files:
test/lint/lint-github-actions.py
📚 Learning: 2025-10-05T20:38:28.457Z
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Applied to files:
test/lint/lint-github-actions.py
📚 Learning: 2025-11-04T18:24:27.241Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Applied to files:
test/lint/lint-github-actions.py
📚 Learning: 2025-07-23T09:30:34.631Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.631Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
Applied to files:
test/lint/lint-github-actions.py
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Dash extends Bitcoin Core through composition with minimal changes to the Bitcoin Core foundation
Applied to files:
test/lint/lint-github-actions.py
📚 Learning: 2025-08-11T17:16:36.654Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Applied to files:
test/lint/lint-github-actions.py
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h} : Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Applied to files:
test/lint/lint-github-actions.py
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/**/*.{cpp,h,hpp,cc} : Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Applied to files:
test/lint/lint-github-actions.py
🪛 Flake8 (7.3.0)
test/lint/lint-github-actions.py
[error] 71-71: f-string is missing placeholders
(F541)
[error] 76-76: f-string is missing placeholders
(F541)
🪛 Ruff (0.14.6)
test/lint/lint-github-actions.py
53-53: Starting a process with a partial executable path
(S607)
71-71: f-string without any placeholders
Remove extraneous f prefix
(F541)
76-76: f-string without any placeholders
Remove extraneous f prefix
(F541)
102-102: subprocess call: check for execution of untrusted input
(S603)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Lint / Run linters
- GitHub Check: Build container / Build container
🔇 Additional comments (3)
test/lint/lint-github-actions.py (3)
50-61:check_zizmor_installbehavior looks good.The existence check via
zizmor --versionand gracefulFileNotFoundErrorhandling (exit 0, clear install hint) is reasonable for CI usage. No changes needed here.
16-47: DISABLED/IGNORED configuration is well-documented and scoped.The split between globally disabled audits and narrowly scoped ignored findings, plus the inline comments explaining each case, keeps the zizmor config understandable and auditable. No changes needed here.
89-110: Tempfile handling and cleanup are appropriate.Using
NamedTemporaryFile(delete=False)with an explicitos.unlinkin afinallyblock is a reasonable pattern here; it avoids Windows handle issues and ensures cleanup even when zizmor fails.
|
This pull request has conflicts, please rebase. |
Review GateCommit:
|
Move all ${{ }} expressions from run: script bodies to env: blocks.
This prevents potential script injection attacks by ensuring that
expression values are passed as environment variables rather than
being interpolated directly into shell scripts.
While most of these expressions come from trusted sources (workflow_call
inputs with hardcoded values, step outputs, safe github context fields),
this change provides structural safety as a defense-in-depth measure.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
- Add persist-credentials: false to all checkout actions to prevent credential persistence in .git/config (artipacked vulnerability) - Add explicit minimal permissions blocks to workflows that were using defaults (clang-diff-format, prevent-master-pr, release_docker_hub, semantic-pull-request) - Scope permissions to job level in guix-build.yml where possible (build-image needs packages:write, build needs id-token/attestations) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add lint-github-actions.py to check workflows for security issues using zizmor. The linter: - Checks for template injection, dangerous triggers, excessive permissions, credential persistence, and other security issues - Skips gracefully if zizmor is not installed - Disables unpinned-uses and unpinned-images audits (not a priority) - Ignores specific findings that are intentional or false positives: - dangerous-triggers: pull_request_target used with proper safeguards - template-injection: workflow_call inputs from hardcoded callers - excessive-permissions: required for reusable workflow architecture Install zizmor with: pip install zizmor 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Install zizmor==1.17.0 in the CI container so lint-github-actions.py can run as part of the lint suite. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Co-authored-by: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com>
PastaPastaPasta
force-pushed
the
fix/workflow-script-injection-hardening
branch
from
fcae891 to
4374c76
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/workflows/build-depends.yml:
- Around line 131-139: The env references HOST and DEP_OPTS are using a
non-existent step id "setup" (steps.setup.outputs.*) which is invalid across
jobs; update the job to pull these values from the producing job outputs using
needs.check-cache.outputs.HOST and needs.check-cache.outputs.DEP_OPTS (i.e.
replace ${{ steps.setup.outputs.HOST }} and ${{ steps.setup.outputs.DEP_OPTS }}
with ${{ needs.check-cache.outputs.HOST }} and ${{
needs.check-cache.outputs.DEP_OPTS }}), and ensure the job declares needs:
check-cache so those outputs are available for the env block and subsequent run
steps that reference HOST and DEP_OPTS.
In @.github/workflows/build-src.yml:
- Around line 154-160: The workflow step incorrectly hardcodes
BASE_OUTDIR="/output", which overrides the path logic in ci/dash/matrix.sh and
ci/test/00_setup_env.sh; remove the BASE_OUTDIR="/output" assignment so
ci/dash/test_unittests.sh can source ci/dash/matrix.sh and let the existing
setup scripts derive BASE_OUTDIR, or if you truly need a shared output location,
pass that path explicitly into the step as an environment variable (e.g., set
BASE_OUTDIR from a workflow input or a prior job output) rather than rebinding
it inline.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4f8d210c-eaeb-4269-99c8-f9282fc50618
📒 Files selected for processing (14)
.github/workflows/build-container.yml.github/workflows/build-depends.yml.github/workflows/build-src.yml.github/workflows/build.yml.github/workflows/clang-diff-format.yml.github/workflows/guix-build.yml.github/workflows/lint.yml.github/workflows/merge-check.yml.github/workflows/predict-conflicts.yml.github/workflows/prevent-master-pr.yml.github/workflows/release_docker_hub.yml.github/workflows/semantic-pull-request.yml.github/workflows/test-src.ymlcontrib/containers/ci/ci-slim.Dockerfile
✅ Files skipped from review due to trivial changes (6)
- .github/workflows/semantic-pull-request.yml
- .github/workflows/predict-conflicts.yml
- .github/workflows/prevent-master-pr.yml
- contrib/containers/ci/ci-slim.Dockerfile
- .github/workflows/clang-diff-format.yml
- .github/workflows/release_docker_hub.yml
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/lint.yml
- .github/workflows/build.yml
- .github/workflows/guix-build.yml
- .github/workflows/build-container.yml
- .github/workflows/merge-check.yml
- Add persist-credentials: false to all checkout steps - Move template-injection-flagged inputs to env vars - Fix cross-job step reference in build-depends.yml - Remove stale BASE_OUTDIR in build-src.yml unit tests - Add permissions block to cache-depends-sources.yml - Update zizmor ignore list for remaining false positives
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build-depends.yml (1)
56-69:⚠️ Potential issue | 🟠 MajorUpdate
restore-keysto include${RUNS_ON}component matching the cache key structure.Line 68 sets the cache key as
depends-${DOCKERFILE_HASH}-${RUNS_ON}-${BUILD_TARGET}-${DEP_HASH}-${PACKAGES_HASH}, but the fallback prefix at line 131 isdepends-${DOCKERFILE_HASH}-${BUILD_TARGET}-. The missing${RUNS_ON}component means the fallback prefix won't match the new key structure, breaking cache restores on exact-key misses.Suggested fix
- name: Restore cached depends uses: actions/cache/restore@v5 with: path: depends/built/${{ needs.check-cache.outputs.host }} key: ${{ needs.check-cache.outputs.cache-key }} restore-keys: | - depends-${{ hashFiles('contrib/containers/ci/ci.Dockerfile', 'contrib/containers/ci/ci-slim.Dockerfile') }}-${{ inputs.build-target }}- + depends-${{ hashFiles('contrib/containers/ci/ci.Dockerfile', 'contrib/containers/ci/ci-slim.Dockerfile') }}-${{ inputs.runs-on }}-${{ inputs.build-target }}-🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-depends.yml around lines 56 - 69, The cache restore fallback prefix doesn't include the RUNS_ON component so it won't match the CACHE_KEY format built by CACHE_KEY="depends-${DOCKERFILE_HASH}-${RUNS_ON}-${BUILD_TARGET}-${DEP_HASH}-${PACKAGES_HASH}"; update the restore-keys entry that currently uses "depends-${DOCKERFILE_HASH}-${BUILD_TARGET}-" to include RUNS_ON, e.g. "depends-${DOCKERFILE_HASH}-${RUNS_ON}-${BUILD_TARGET}-", so restore-keys matches the CACHE_KEY structure (adjust the restore-keys string where restore-keys is defined to reference RUNS_ON).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/lint/lint-github-actions.py`:
- Around line 53-64: The current check_zizmor_install() silently exits 0 if
'zizmor' is missing and calls subprocess.run with a partial executable, which
triggers Ruff S607 and lets CI pass without the audit; change it to use
shutil.which('zizmor') to resolve the full path, use that full path (not the
string 'zizmor') in subprocess.run calls (including the other usages around the
96-105 block), and when the tool is absent detect CI (e.g. os.getenv('CI') or
os.getenv('GITHUB_ACTIONS')) to print the install instructions and exit(1) so CI
fails instead of exiting 0. Ensure all subprocess.run invocations that currently
pass ['zizmor', ...] use the resolved full-path variable.
---
Outside diff comments:
In @.github/workflows/build-depends.yml:
- Around line 56-69: The cache restore fallback prefix doesn't include the
RUNS_ON component so it won't match the CACHE_KEY format built by
CACHE_KEY="depends-${DOCKERFILE_HASH}-${RUNS_ON}-${BUILD_TARGET}-${DEP_HASH}-${PACKAGES_HASH}";
update the restore-keys entry that currently uses
"depends-${DOCKERFILE_HASH}-${BUILD_TARGET}-" to include RUNS_ON, e.g.
"depends-${DOCKERFILE_HASH}-${RUNS_ON}-${BUILD_TARGET}-", so restore-keys
matches the CACHE_KEY structure (adjust the restore-keys string where
restore-keys is defined to reference RUNS_ON).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6f79ace5-4d87-48d6-a50e-5c208e81683c
📒 Files selected for processing (6)
.github/workflows/build-container.yml.github/workflows/build-depends.yml.github/workflows/build-src.yml.github/workflows/build.yml.github/workflows/cache-depends-sources.ymltest/lint/lint-github-actions.py
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/build-container.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/build.yml
| def check_zizmor_install(): | ||
| try: | ||
| subprocess.run( | ||
| ['zizmor', '--version'], | ||
| stdout=subprocess.DEVNULL, | ||
| stderr=subprocess.DEVNULL, | ||
| check=True | ||
| ) | ||
| except FileNotFoundError: | ||
| print('Skipping GitHub Actions linting since zizmor is not installed.') | ||
| print('Install with: pip install zizmor') | ||
| sys.exit(0) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd test/lint && wc -l lint-github-actions.pyRepository: dashpay/dash
Length of output: 82
🏁 Script executed:
cd test/lint && cat -n lint-github-actions.py | head -120Repository: dashpay/dash
Length of output: 4331
🏁 Script executed:
find . -name "pyproject.toml" -o -name ".ruff.toml" -o -name "ruff.toml" | head -5Repository: dashpay/dash
Length of output: 96
🏁 Script executed:
rg "S603|S607" --type py test/lint/lint-github-actions.pyRepository: dashpay/dash
Length of output: 38
🏁 Script executed:
rg "noqa.*S603" --type pyRepository: dashpay/dash
Length of output: 38
🏁 Script executed:
cat ./ruff.tomlRepository: dashpay/dash
Length of output: 1133
🏁 Script executed:
python3 << 'EOF'
# Check what S603 and S607 mean in Ruff/Bandit context
# S603: subprocess call with shell=True
# S607: Starting a process with a partial executable
rules = {
'S603': 'subprocess call with shell=True - Executing shell command from a list',
'S607': 'Starting a process with a partial executable'
}
for rule, desc in rules.items():
print(f"{rule}: {desc}")
EOFRepository: dashpay/dash
Length of output: 182
🏁 Script executed:
# Let's verify the actual subprocess calls in the file
cd test/lint && rg "subprocess\.run" -A 3 lint-github-actions.pyRepository: dashpay/dash
Length of output: 330
Don't silently skip zizmor in CI, and fix the subprocess lint findings.
Line 64 exits with code 0 when zizmor is missing, allowing CI to pass silently without running the security audit. The subprocess.run() calls at lines 56 and 105 use partial executables ('zizmor') which trigger Ruff's S607 rule. Use shutil.which() to obtain the full path, and detect CI environments to exit(1) instead of exit(0) when the tool is unavailable.
Suggested fix
+import shutil
import subprocess
import sys
import tempfile
import os
@@
-def check_zizmor_install():
- try:
- subprocess.run(
- ['zizmor', '--version'],
- stdout=subprocess.DEVNULL,
- stderr=subprocess.DEVNULL,
- check=True
- )
- except FileNotFoundError:
- print('Skipping GitHub Actions linting since zizmor is not installed.')
- print('Install with: pip install zizmor')
- sys.exit(0)
+def get_zizmor_path():
+ zizmor_path = shutil.which('zizmor')
+ if zizmor_path is None:
+ if os.environ.get('CI') or os.environ.get('GITHUB_ACTIONS'):
+ print('zizmor is required in CI but was not found on PATH.')
+ sys.exit(1)
+ print('Skipping GitHub Actions linting since zizmor is not installed.')
+ print('Install with: pip install zizmor')
+ sys.exit(0)
+ return zizmor_path
@@
def main():
- check_zizmor_install()
+ zizmor_path = get_zizmor_path()
+ subprocess.run(
+ [zizmor_path, '--version'],
+ stdout=subprocess.DEVNULL,
+ stderr=subprocess.DEVNULL,
+ check=True,
+ ) # noqa: S607
@@
zizmor_cmd = [
- 'zizmor',
+ zizmor_path,
'--config', config_path,
'.github/workflows/',
]
@@
- result = subprocess.run(zizmor_cmd)
+ result = subprocess.run(zizmor_cmd) # noqa: S607Also applies to: 96-105
🧰 Tools
🪛 Ruff (0.15.9)
[error] 56-56: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/lint/lint-github-actions.py` around lines 53 - 64, The current
check_zizmor_install() silently exits 0 if 'zizmor' is missing and calls
subprocess.run with a partial executable, which triggers Ruff S607 and lets CI
pass without the audit; change it to use shutil.which('zizmor') to resolve the
full path, use that full path (not the string 'zizmor') in subprocess.run calls
(including the other usages around the 96-105 block), and when the tool is
absent detect CI (e.g. os.getenv('CI') or os.getenv('GITHUB_ACTIONS')) to print
the install instructions and exit(1) so CI fails instead of exiting 0. Ensure
all subprocess.run invocations that currently pass ['zizmor', ...] use the
resolved full-path variable.
|
This pull request has conflicts, please rebase. |
4 participants
Issue being fixed or feature implemented
We should lint our actions
What was done?
Implement linter, fix some issues it identified. None of the issues fixed seem to be exploitable or true issues, more just smells.
How Has This Been Tested?
Ran linter
Breaking Changes
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.