ci(security): Semgrep SAST gate (report-only) (#1797) - #1822
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between c75ef32729ef4d9320667edfdcb4db7f2c748816 and dbc212e. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a GitHub Actions workflow for report-only Semgrep scans on pull requests, nightly runs, and manual dispatches. It excludes build artifacts, summarizes SARIF findings, and uploads results to GitHub Code Scanning when permitted. ChangesSemgrep SAST
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant SemgrepContainer
participant JobSummary
participant CodeScanning
GitHubActions->>SemgrepContainer: Run Semgrep and write semgrep.sarif
GitHubActions->>JobSummary: Render severity and rule counts
GitHubActions->>CodeScanning: Upload SARIF for eligible runs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
0b3ce82 to
4e1a86c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/semgrep.yml (2)
95-98: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPrefer
!cancelled()overalways()to avoid failures on workflow cancellation.Using
always()forces these steps to execute even if the workflow is aborted (e.g., superseded by a newer commit or cancelled manually). If the run is cancelled before thesemgrep.sariffile is created, the upload step will fail with a missing-file error and surface a confusing red X. Replacingalways()with!cancelled()ensures these steps still run if the scan itself fails (such as when you later add--error), but safely skips them if the workflow was cancelled.
.github/workflows/semgrep.yml#L95-L98: change${{ always() && ... }}to${{ !cancelled() && ... }}in the SARIF upload step..github/workflows/semgrep.yml#L64-L65: changealways()to!cancelled()in the summary step.🤖 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 @.github/workflows/semgrep.yml around lines 95 - 98, Replace always() with !cancelled() in both the summary step at .github/workflows/semgrep.yml lines 64-65 and the SARIF upload step at lines 95-98, preserving the existing conditions so both steps continue after scan failures but skip cancelled workflow runs.
64-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
!cancelled()overalways()to avoid running on workflow cancellation.Using
always()forces this step to run even if the workflow is cancelled by a user or by the concurrency group. It is safer to useif: !cancelled()so the step runs on both success and failure (e.g., if you later add--errorto the scan step), but skips execution if the run was aborted.♻️ Proposed fix
- name: Summarize findings in the job summary - if: always() + if: !cancelled() # Null-guarded append (repo convention, cf. e2e-bronze-to-api.yml). Renders a🤖 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 @.github/workflows/semgrep.yml around lines 64 - 65, Update the “Summarize findings in the job summary” step condition from always() to !cancelled(). Preserve execution after successful or failed scans while preventing the step from running when the workflow is cancelled.
🤖 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 @.github/workflows/semgrep.yml:
- Around line 95-98: Replace always() with !cancelled() in both the summary step
at .github/workflows/semgrep.yml lines 64-65 and the SARIF upload step at lines
95-98, preserving the existing conditions so both steps continue after scan
failures but skip cancelled workflow runs.
- Around line 64-65: Update the “Summarize findings in the job summary” step
condition from always() to !cancelled(). Preserve execution after successful or
failed scans while preventing the step from running when the workflow is
cancelled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a3e96c8-055f-4138-8c85-fe9a4af320da
📥 Commits
Reviewing files that changed from the base of the PR and between 067d808 and 85f127b2a170bba665b57a47584fc344bcf01657.
📒 Files selected for processing (2)
.github/workflows/semgrep.yml.semgrepignore
Standalone semgrep.yml running p/default (secrets excluded — TruffleHog owns those) in report-only mode: findings upload to Code Scanning (SARIF) and a job-summary count, but no --error so the check never blocks. PR + nightly + dispatch triggers, digest-pinned image, SHA-pinned actions. Adds .semgrepignore for build/dep artifacts and waives the known JWT ValidateLifetime finding inline (#346). Split from #1463; part of #1478. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
- Bump actions/checkout v4.2.2 -> v5.0.1 and codeql-action/upload-sarif v3.32.0 -> v4.32.6 (SHA-pinned) to clear the Node-20 deprecation and the CodeQL Action v3 sunset warnings from the first run. - Switch the JWT #346 waiver from an inline rule-id nosemgrep to a bare // nosemgrep: under the full p/default ruleset the fully-namespaced rule id does not match, so no suppression was emitted and the alert stayed open. The bare form reliably emits SARIF suppressions[inSource], which Code Scanning honors as a dismissed alert. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Replace the bare finding count with a severity table plus a collapsible per-rule breakdown rendered from the SARIF (stdlib only, no jq). Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
p/default has thin Rust/C# security coverage (0 findings across 184 Rust files); add the dedicated language packs so the actual backend code is scanned, not just CI/YAML. Surfaces real Rust (unsafe-usage, temp-dir) and C# (unsigned-security-token) findings p/default missed. Still report-only. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…component) Match the org semgrep-scan component's config=auto (per-language auto-detection via the registry). Drops the explicit p/rust/p/csharp/p/python packs and the --metrics off flag (auto requires metrics on to fetch its ruleset). Still report-only, SARIF -> Code Scanning + job summary. Note: on this repo auto currently yields no Rust findings (same coverage as p/default); revisit language packs if Rust/C# depth is needed. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
auto matches the org component but has thin Rust/C# coverage (0 Rust findings on this repo). Layer the dedicated language packs on top so the Rust backend and C# identity service are actually scanned (unsafe-usage, temp-dir, unsigned-security-token). Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Bump the digest-pinned Semgrep image to the latest release (was inherited from the closed PR #1463; org GitLab component is on 1.154.x). Config output verified unchanged (auto + p/rust + p/csharp). Report-only. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Turn the 'Security -> Code scanning' text in the job summary into a clickable link filtered to the current branch (GITHUB_HEAD_REF on PRs, else GITHUB_REF_NAME). Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…nch:) PR runs upload SARIF against the merge ref, so alerts live under pr:<n>, not the branch ref — the branch-filtered link showed 0. Use pr:<n> on pull_request events and branch:<name> on push/schedule/dispatch. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
a17ab5c to
dbc212e
Compare
What
Stands up Semgrep (SAST) as its own report-only CI gate, split out of the
closed omnibus PR #1463 so the baseline can be triaged and ratcheted to blocking
independently (per the security-scanner umbrella #1478).
.github/workflows/semgrep.yml— singlesastjob:semgrep scan --config p/default --exclude-rule generic.secrets.security.detected-generic-secret(secret detection is owned by the separate TruffleHog gate).
--error, so findings surface but never block.main+ nightlyschedule(03:27 UTC) +workflow_dispatch; no path filter.docker runonubuntu-latest(socheckout/upload-sarifkeep the host runner's Node); SHA-pinned actions; job-scoped
security-events: write;concurrencycancel-in-progress; fork-PR-safe SARIF upload..semgrepignore— prunes build/dependency artifacts (target/,.venv/,node_modules/,dist/);application and test source stay in scope.
Program.cs— the pre-existing JWTValidateLifetime = falsefinding is waived inline(
// nosemgrep: …+#346), the one known high-severity item in the baseline.Baseline (first
p/defaultrun: 124 findings)Report-only on purpose — the baseline is not clean yet. Dominant category is 109
github-actions-mutable-action-tagfindings in the existing workflows (a repo-wideSHA-pin effort, tracked separately), plus ~13 product/tooling findings and 2 Dockerfile
missing-userfindings. Triage happens in Code Scanning after this lands.Exit condition (separate follow-up PR)
Flip to blocking by adding
--errorand markingsasta required check once the baseline isclean (zero un-waived findings) — which depends on the mutable-tag follow-up + triaging the
remaining findings. Not part of this PR.
Test plan
actionlintclean; the new workflow self-scans to 0p/defaultfindings (fully pinned).Closes #1797.
Part of #1478. Split from #1463.
Summary by CodeRabbit
📊 Reports (branch
semgrep-collection)Report-only baseline: findings surface here but do not block. See the Code-scanning tab for per-alert detail and dismissal; the run's Summary tab for the severity + per-rule breakdown.