Skip to content

CI: suppress shell:S4830 false positive and scope GHA permissions to job level#1229

Open
rgsl888prabhu wants to merge 2 commits into
mainfrom
fix/sonar-tls-suppress-self-hosted-test
Open

CI: suppress shell:S4830 false positive and scope GHA permissions to job level#1229
rgsl888prabhu wants to merge 2 commits into
mainfrom
fix/sonar-tls-suppress-self-hosted-test

Conversation

@rgsl888prabhu
Copy link
Copy Markdown
Collaborator

@rgsl888prabhu rgsl888prabhu commented May 15, 2026

Summary

Closes 6 SonarQube findings on main — 1 CRITICAL VULN and 5 MAJOR VULNs — without changing runtime behavior.

ci/test_self_hosted_service.sh — suppress shell:S4830

The curl -k on 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 (subsequent cuopt_sh calls do validate using the test CA in $CLIENT_CERT). Added a # NOSONAR marker with rationale.

.github/workflows/build_test_publish_images.yaml — job-scoped permissions

Moved the workflow-level permissions: block to per-job blocks (rule githubactions:S8264/S8233). After reading the two reusable workflows (build_images.yaml, test_images.yaml), every job only does actions/checkout + DockerHub/NGC logins via username/password secrets — no OIDC, no GHCR pull, no artifact download, no PR API. So each job is reduced to contents: read only, dropping unused actions: read, id-token: write, packages: read, pull-requests: read.

rgsl888prabhu and others added 2 commits May 15, 2026 11:29
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>
@rgsl888prabhu rgsl888prabhu requested a review from a team as a code owner May 15, 2026 17:23
@rgsl888prabhu rgsl888prabhu requested a review from msarahan May 15, 2026 17:23
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 15, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

This PR tightens GitHub Actions workflow permissions by replacing a broad top-level permissions declaration with minimal per-job contents: read grants, and adds an inline comment documenting CI certificate generation in a test script. Both changes are configuration and documentation updates with no functional logic modifications.

Changes

CI Configuration and Documentation

Layer / File(s) Summary
GitHub Actions job-level permissions hardening
.github/workflows/build_test_publish_images.yaml
Removes the broad top-level permissions block and adds explicit job-level permissions: contents: read to compute-matrix, build-images, build-cuopt-multiarch-manifest, and test-images jobs, establishing least-privilege token scopes.
Self-hosted service test documentation
ci/test_self_hosted_service.sh
Appends an inline comment to the server_status curl health-check command documenting that the self-signed certificate is generated locally for CI.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely summarizes the two main changes: suppressing a shell linting false positive and scoping GitHub Actions permissions to job level.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The pull request description is directly related to the changeset, clearly explaining the rationale for both modified files and the SonarQube findings being addressed.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/sonar-tls-suppress-self-hosted-test

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
ci/test_self_hosted_service.sh (1)

82-82: 💤 Low value

Minor 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 -k is 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

📥 Commits

Reviewing files that changed from the base of the PR and between 72ba054 and ba4554f.

📒 Files selected for processing (2)
  • .github/workflows/build_test_publish_images.yaml
  • ci/test_self_hosted_service.sh

@rgsl888prabhu rgsl888prabhu self-assigned this May 15, 2026
@rgsl888prabhu rgsl888prabhu added non-breaking Introduces a non-breaking change improvement Improves an existing functionality labels May 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improves an existing functionality non-breaking Introduces a non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants