Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
f81b692 to
65db296
Compare
psof
left a comment
There was a problem hiding this comment.
Good foundation. Supply chain hygiene is solid. Two things in inline comments worth addressing before this becomes a pattern others copy.
The centralized rule distribution architecture is the right call. Getting the pinning story right on the security-github-actions content fetch is the one structural thing to resolve before this scales.
| - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 | ||
| with: | ||
| repository: grafana/security-github-actions | ||
| ref: ${{ github.repository == 'grafana/security-github-actions' && (github.head_ref || github.ref_name) || '' }} |
There was a problem hiding this comment.
Here, the action is pinned but the content isn't. When the workflow runs in grafana/security-github-actions: ref is the PR branch or current branch (so it allows testing rule changes in the repo etc).
When the workflow runs in any other repo (e.g. grafana/some-app): the condition is false, so ref is '' (empty string). With ref: '', actions/checkout uses the default branch of grafana/security-github-actions (main). So:
- There is no built-in rollback: you can’t point “all consumers” at a “last known good” without changing something (e.g. the ref in the workflow).
- Any push to the main branch of
security-github-actions(mistake or compromise) is immediately what every other repo’s PRs use. No separate “release” or review in the consuming repos.
These points are even more painful if this workflow is ever used in an enforcing/block mode.
It is a good idea to pin the ref to a release tag or commit for consuming repos. Then updates are intentional, you change the tag/SHA in the workflow (or in a shared template) when you want to roll out a new version. Additionally, a bad push to main does not automatically affect consumers until something explicitly updates that ref.
There was a problem hiding this comment.
Make sense. I can do this in a future PR with the config file in place
| GITHUB_BRANCH: ${{github.head_ref || github.ref_name}} | ||
| run: | | ||
| set +e | ||
| semgrep scan --error --json --config security-github-actions/semgrep/custom-rules.yaml > /tmp/semgrep-results.json 2>/dev/null |
There was a problem hiding this comment.
2>/dev/null on the semgrep step hides all stderr. When the run fails (missing rules file, bad config, container issue, etc.) we only get a non-zero exit code and no error message, so it’s hard to see why it failed.
Because the step also has continue-on-error: true, the workflow keeps going on failure. Combined with hidden stderr, it’s easy to miss that semgrep failed at all—the run can look green while the scan never actually ran or produced valid results.
Suggestion: Remove the redirect so stderr appears in the job log, or redirect it to a file and cat that file when EXIT_CODE -ne 0 so failures are visible in the Actions log.
|
Agree with Polydoros' comments on the workflow. The SemGrep code itself looks good to me, if we'd rather have this be managed via semgrep cloud and run as a comment/block rule on PRs via their integrations (which are what we're using right now for rolling semgrep out across the company), I am happy to add it there instead. |
There was a problem hiding this comment.
Pull request overview
Adds an org-focused Semgrep OSS scan to run on pull requests, with custom rules and a results formatter so findings can be surfaced directly on PRs and optionally fail the workflow based on severity.
Changes:
- Adds a new GitHub Actions workflow to run Semgrep in a container on
pull_requestand post findings as a PR comment. - Introduces an org-wide Semgrep rule to flag
actions/create-github-app-tokenusage and point to the approved alternative. - Adds a bash formatter that converts Semgrep JSON output into a GitHub-flavored markdown table.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
.github/workflows/semgrep.yaml |
New PR-time Semgrep scan job, including rule checkout and PR commenting/fail policy. |
semgrep/custom-rules.yaml |
Adds a custom Semgrep rule targeting actions/create-github-app-token usage. |
semgrep/format-results.sh |
Formats Semgrep JSON into a markdown table with links back to code locations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - if: steps.semgrep.outputs.has_findings == 'true' | ||
| uses: int128/comment-action@66317511bc86c47bd51e03059040e8a460a167b8 | ||
| with: | ||
| update-if-exists: recreate | ||
| post: | | ||
| ${{ env.SEMGREP_OUTPUT }} | ||
|
|
| echo "has_high_critical=true" >> "$GITHUB_OUTPUT" | ||
| fi | ||
|
|
||
| - if: steps.semgrep.outputs.has_findings == 'true' |
Introduces a reusable Semgrep scanning workflow that runs on pull requests, along with an initial custom rule to deny usage of actions/create-github-app-token and a script to format findings as GitHub-flavored markdown PR comments. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Semgrep Findings1 finding(s) detected.
|
psof
left a comment
There was a problem hiding this comment.
Approving.
This is needed to make progress with the enforcement of Github App Token broker and the pinning improvement will need a follow-up PR since we first need the commit ref anyway
Summary
.github/workflows/semgrep.yaml): Runs Semgrep OSS on every pull request using the official container image (semgrep/semgrep:1.152.0). Findings are posted as a PR comment (recreated on each push) and the job fails if any HIGH or CRITICAL severity issues are found.semgrep/custom-rules.yaml): Adds an org-wide rule that flags usage ofactions/create-github-app-tokenand points to the approved alternative.semgrep/format-results.sh): Converts Semgrep JSON output into a GitHub-flavored markdown table with severity icons, rule names, file links, and messages.Design decisions
grafana/security-github-actionsrepo, so consuming repositories get rule updates automatically without changing their own workflows.security-github-actionsitself, the checkout uses the PR branch (github.head_ref) so rule changes can be tested before merging.--config custom-rules.yamlis used (no--config auto) to keep the scan focused on org-specific rules.Test plan
actions/create-github-app-tokenand verify Semgrep posts a comment with the finding🤖 Generated with Claude Code