CI: suppress shell:S4830 false positive and scope GHA permissions to job level#1229
CI: suppress shell:S4830 false positive and scope GHA permissions to job level#1229rgsl888prabhu wants to merge 2 commits into
Conversation
The `curl -k` on the health-check is intentional: this same script generates the self-signed cert and starts the server two lines earlier, and the test CA is used to validate subsequent `cuopt_sh` calls. There is no real TLS endpoint to trust against. Marks the line with `# NOSONAR` and a rationale so the rule stops firing on every nightly scan. Clears the remaining CRITICAL VULN on `main` (rule shell:S4830). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…4/S8233) Move the `permissions:` block from workflow level to per-job level. Every job in this chain (and in the two reusable workflows it calls, build_images.yaml and test_images.yaml) only performs a checkout plus DockerHub/NGC logins via username/password secrets — there is no OIDC, no GHCR pull, no artifact download, no PR API usage. So each job is reduced to `contents: read` only, dropping the unused workflow-level grants of `actions: read`, `id-token: write`, `packages: read`, and `pull-requests: read`. Clears 5 MAJOR vulnerabilities on `main`: - 4× githubactions:S8264 (read perms at workflow level) - 1× githubactions:S8233 (write perm at workflow level) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR tightens GitHub Actions workflow permissions by replacing a broad top-level ChangesCI Configuration and Documentation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ci/test_self_hosted_service.sh (1)
82-82: 💤 Low valueMinor inaccuracy in NOSONAR rationale.
The comment states the cert is "generated locally by this script," but lines 71-74 show the certificate files are loaded from
python/cuopt_self_hosted/cuopt_sh_client/tests/utils/certs/rather than being generated at runtime. Consider clarifying:-server_status=$(curl -k -sL https://0.0.0.0:$CUOPT_SERVER_PORT/cuopt/health) # NOSONAR — self-signed cert generated locally by this script for CI; not a real TLS endpoint. +server_status=$(curl -k -sL https://0.0.0.0:$CUOPT_SERVER_PORT/cuopt/health) # NOSONAR — self-signed test cert from repo's test fixtures; not a real TLS endpoint.The suppression itself is appropriate since
curl -kis intentional for CI health checks against a local server using test certificates.🤖 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 `@ci/test_self_hosted_service.sh` at line 82, Update the inline NOSONAR comment on the curl invocation that sets server_status to accurately reflect that the script uses pre-generated test certificates from python/cuopt_self_hosted/cuopt_sh_client/tests/utils/certs/ rather than generating them at runtime; keep the curl -k suppression and NOSONAR rationale but change the wording around certificate origin (refer to the server_status curl line) so it correctly documents the source of the test certs.
🤖 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.
Nitpick comments:
In `@ci/test_self_hosted_service.sh`:
- Line 82: Update the inline NOSONAR comment on the curl invocation that sets
server_status to accurately reflect that the script uses pre-generated test
certificates from python/cuopt_self_hosted/cuopt_sh_client/tests/utils/certs/
rather than generating them at runtime; keep the curl -k suppression and NOSONAR
rationale but change the wording around certificate origin (refer to the
server_status curl line) so it correctly documents the source of the test certs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7889d481-adde-4d59-9d66-1cabccb71220
📒 Files selected for processing (2)
.github/workflows/build_test_publish_images.yamlci/test_self_hosted_service.sh
Summary
Closes 6 SonarQube findings on
main— 1 CRITICAL VULN and 5 MAJOR VULNs — without changing runtime behavior.ci/test_self_hosted_service.sh— suppressshell:S4830The
curl -kon the health probe is intentional: this same script generates a self-signed cert and starts the cuOpt server two lines earlier, so there is no real TLS endpoint to validate against (subsequentcuopt_shcalls do validate using the test CA in$CLIENT_CERT). Added a# NOSONARmarker with rationale..github/workflows/build_test_publish_images.yaml— job-scoped permissionsMoved the workflow-level
permissions:block to per-job blocks (rulegithubactions:S8264/S8233). After reading the two reusable workflows (build_images.yaml,test_images.yaml), every job only doesactions/checkout+ DockerHub/NGC logins via username/password secrets — no OIDC, no GHCR pull, no artifact download, no PR API. So each job is reduced tocontents: readonly, dropping unusedactions: read,id-token: write,packages: read,pull-requests: read.