Add Semgrep SAST to CI alongside CodeQL#210
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
|
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. |
m-khan-97
left a comment
There was a problem hiding this comment.
Reviewed. Clean addition and the job wiring is complete — sast-semgrep is added to the needs array, the env block, the summary table, and the final failure-gate condition, so a Semgrep finding will actually fail CI rather than being cosmetic. The continue-on-error: true + if: always() SARIF upload + explicit if: steps.semgrep.outcome == 'failure' gate is the correct pattern (upload happens even on findings; outcome reflects the pre-continue-on-error result). Pinning the registry rulesets explicitly (p/security-audit etc.) instead of --config auto to avoid the semgrep.dev account dependency is the right call, and --metrics=off is good hygiene.
Two notes, neither blocking:
- This PR also adds a non-root
USER openshieldto the Dockerfile — a genuine improvement (and it partially addresses the container-hardening gap I raised on #185), but it's unrelated to "add Semgrep to CI." Fine to keep, just flagging that it's riding along so it's a conscious inclusion.pip installruns before the USER switch and/appis chowned, so it won't break the build. - Semgrep pulls the
p/...rulesets from the registry at run time, so a registry outage fails the job. Acceptable for a SAST gate; worth knowing it's a network dependency if you ever see a spurious red.
Approving.
|
Addressed both notes in 4adb350:
No functional change, docs and a comment only. |
m-khan-97
left a comment
There was a problem hiding this comment.
Re-approving after the two follow-up commits (my earlier approval was auto-dismissed by the new pushes). The two new commits are documentation only — they add the Dockerfile hardening rationale and the Semgrep registry-dependency note, which addresses both non-blocking points from my first review. The functional diff (the sast-semgrep job wiring + the non-root USER openshield Dockerfile change) is byte-identical to what I already reviewed and what previously passed CI.
CI is re-running from the new commits (pending as I write this); merge is gated on it anyway. Approving on the code — good to merge once checks go green.
|
Merging this on priority, as the CODEQL has been disabled, thanks for the review @m-khan-97. Thanks to @ritiksah141 for quickly spinning this up! |
Summary
Adds Semgrep as a second, open-source SAST layer in CI, running alongside the existing CodeQL workflow, which stays unchanged.
What changed
CI (Semgrep)
sast-semgrepjob inci.yml: puresemgrep scanCLI (notsemgrep ci, which is semgrep.dev integrated), public Registry rulesets (p/security-audit,p/owasp-top-ten,p/python,p/javascript), no account, no login,--metrics=offdisables telemetry.github/codeql-action/upload-sarifrelease already used bycodeql.yml, so findings appear in Security → Code scanning alongside CodeQL's.ci-summarythe same way every other job is:needsarray, env block, checks table, fail-gate condition.Dockerfile
dockerfile.security.missing-userrule flagged the image running as root. Added a non-rootopenshieldsystem user and switchedUSERbeforeCMD. Required to make the new gate pass, not part of the original CI-wiring scope but necessary.Docs
docs/ci-pipeline.md: new SAST (Semgrep) row in the jobs table, a new local-repro subsection, a tooling-decisions bullet explaining the account-free choice, and a fixing-common-failures row fornosemgrepsuppression.CHANGELOG.md: one-line entry under Unreleased/Added.Why
GitHub's Code Quality preview product moves to paid general availability on July 20, 2026. It is a separate, admin-only Settings toggle, distinct from this repo's existing
codeql.ymlworkflow, and disabling it needs org-admin access outside the scope of a PR (raised with the maintainer separately). Independent of that billing technicality, the maintainer's direction is to shift security tooling toward open source, so this adds Semgrep as a fully self-hosted, account-free SAST layer rather than touching CodeQL, which stays as is.Notes for reviewers
p/secretsruleset deliberately excluded. Gitleaks already owns secret detection in thesecret-scanjob, and adding a second, differently-tuned secrets ruleset on day one risks duplicate noise. Can be added later as its own decision..semgrepignoreadded. Semgrep's built-in default ignores already covernode_modules/,dist/,build/,.venv/,test(s)/, and it auto-respects this repo's.gitignore(venv/already excluded). The--excludeflags in the CI command are kept only for in-workflow visibility.nosemgrepsuppressions inscanner/nvd_client.pyandscripts/render_deploy.py(added in a prior one-off Semgrep pass) still resolve correctly under the new ruleset combination, confirmed locally. They show up in the uploaded SARIF as suppressed findings, since the SARIF spec keeps suppressed results visible for audit trail, but they do not block the gate.Local verification (all green)
Ran the exact CI command locally:
semgrep scan --config p/security-audit --config p/owasp-top-ten --config p/python --config p/javascript --exclude venv --exclude frontend/node_modules --exclude frontend/dist --metrics=off --sarif --output semgrep.sarif --error .. Exit code 0 after the Dockerfile fix, with only the two pre-suppressed findings remaining. Dockerfile also checked withhadolint, no issues. Workflow YAML confirmed to parse withpython3 -c "import yaml; yaml.safe_load(open('.github/workflows/ci.yml'))".Related issue
Closes #209
Files to review
.github/workflows/ci.ymlsast-semgrepjob plus the threeci-summarywiring pointsDockerfileUSERadded to satisfy the new Semgrep gatedocs/ci-pipeline.mdCHANGELOG.md