Skip to content

Pin Semgrep CI image by digest and scope security-events to the upload job (CWE-829) #31

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
07souravkunda wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Pin Semgrep CI image by digest and scope security-events to the upload job (CWE-829) #31

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1fc44b3
Pin Semgrep CI image by digest and drop security-events from the cont…
07souravkunda Aug 12, 2026
8b08eb9
Add a 7-day cooldown to the Dependabot config
07souravkunda Aug 12, 2026
233e0b0
Don't turn the upload job red when no SARIF was ever produced
07souravkunda Aug 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions .github/dependabot.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# Keeps the digest/SHA pins in .github/workflows/ from going stale.
#
# NOTE: Dependabot cannot bump the `container: image:` digest in a workflow file — its
# `docker` ecosystem only parses Dockerfiles, Kubernetes manifests and Helm values
# (dependabot/dependabot-core#5819), and `github-actions` only covers `uses:` refs.
# The Semgrep image digest in Semgrep.yml must therefore be refreshed manually (the
# command is in a comment next to the pin), or by adopting Renovate, which does support
# workflow container digests.
version: 2
updates:
- package-ecosystem: "github-actions"
directory: "/"
schedule:
interval: "weekly"
open-pull-requests-limit: 5
Comment thread
github-advanced-security[bot] marked this conversation as resolved.
Fixed
# Don't propose a version the day it is published: a compromised or hijacked release
# is usually caught and yanked within a few days. Same reasoning as the digest pin.
cooldown:
default-days: 7
56 changes: 50 additions & 6 deletions .github/workflows/Semgrep.yml
View file
Open in desktop

@07souravkunda 07souravkunda Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[blocking] The two component findings this change closes are still unrouted on the tracker — including the one whose remediation this file is.

The change itself is correct and I could verify every claim behind it (see the review summary). The gap is on the tracker side: the finding that asks for the digest pin on this exact file is still sitting in New Item with no record that a fix exists, and its sibling is in the same state. Both are still assigned to a different engineer, so from their side nothing has happened.

Evidence

  • Chain-breaker finding (digest pin on .github/workflows/Semgrep.yml) — status New Item, assignee unchanged, only comment is a June ETA note from security. Nothing links it to this PR.
  • Sibling finding (composer.lock) — status New Item, no comment. The fix write-up argues at length that it cannot be fixed as worded (Composer 2.9 refuses phpunit/phpunit: 4.6.* under block-insecure; forcing a lock pins CVE-2026-24765 high + 3 symfony/yaml lows). That reasoning is sound and I re-checked it — but it lives only in the parent's comment and in this PR body. The ticket a human will actually triage says nothing.
  • The parent's completion comment does say "Once merged, LOC-6931 and LOC-6925 can both close; LOC-6927 needs the separate PHPUnit decision" — but a sentence on the parent is not a routing record on the siblings. Whoever picks either one up next sees an untouched ticket.

Fix

  • On the chain-breaker finding: a completion comment naming this PR as the fix ("covered by the chain-breaker fix in PR Pin Semgrep CI image by digest and scope security-events to the upload job (CWE-829) #31"), so it closes with the parent.
  • On the composer.lock finding: its own wont-do / dispute comment carrying the audit evidence already gathered (composer-audit-if-locked.txt, composer.lock.rejected-evidence) and the follow-up ask (migrate tests/LocalTest.php off the PHPUnit 4.6 API), so it can be closed as Won't Do rather than left open with no explanation.

No change to this diff is needed for either.

@07souravkunda 07souravkunda Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done — both siblings are routed on the tracker now, not just referenced from the parent.

Chain-breaker (LOC-6925 · F-009) — assigned to the operator, moved to Dev in Progress, labelled locsec-fixed, and given a full completion comment naming this PR as its remediation ("Covered by the chain-breaker fix in PR #31"). It carries its own assessment rather than a pointer: its own CVSS mismatch (7.5 stated vs 8.0 recomputed from its own vector), the reachability evidence (the three consecutive scheduled runs that executed the mutable ref), the digest provenance, and the run ids that prove the fix. It closes with the parent.

Sibling (LOC-6927 · F-011) — assigned, moved to Dev in Progress, labelled locsec-wontfix, and given its own won't-do assessment (no-fix-available, because the prescribed remediation is what's unavailable). It carries the audit evidence in full: the exact composer 2.9 block-insecure failure text, the 4 advisories a forced lock would pin (phpunit 4.6.10 → CVE-2026-24765 HIGH, symfony/yaml v3.4.47 → three lows, plus the abandoned mock-objects package), the pointer to composer.lock.rejected-evidence / composer-audit-if-locked.txt, and the follow-up with owner and auto-resolve condition. Recommended close as Won't Do — proposed, not accepted; that call stays with a human.

Three corrections to that ticket went in with it, since a human triaging it would otherwise inherit them: its score disagrees with its own vector (5.9 vs 6.5); its data-flow step 3 rests on the dead Travis composer install; and its Impact overstates blast radius as "all consumers of this library" when Composer never installs a library's require-dev downstream. I also named the root cause the report missed — .gitignore:3 ignores composer.lock deliberately, so any real fix has to change that line.

Also recorded the won't-do as a row in .claude/knowledge/workspace/false-positives.md (Table 2), per the dispute-skill requirement, so the next agent that meets this constraint doesn't re-derive it.

You were right that a sentence on the parent isn't a routing record. Agreed and fixed — no change to this diff.

Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,25 @@ permissions:
jobs:
semgrep:
# User definable name of this GitHub Actions job.
# This job runs third-party code (the Semgrep container), so it is granted
# `contents: read` only. SARIF upload — which needs `security-events: write` — is
# deliberately isolated in the `upload-sarif` job below, so a compromised image
# cannot write to the repository's code-scanning dashboard.
permissions:
contents: read # for actions/checkout to fetch code
security-events: write # for github/codeql-action/upload-sarif to upload SARIF results
name: semgrep/ci
# If you are self-hosting, change the following `runs-on` value:
name: semgrep/ci
# If you are self-hosting, change the following `runs-on` value:
runs-on: ubuntu-latest

container:
# A Docker image with Semgrep installed. Do not change this.
image: returntocorp/semgrep:1.166.0
# Pinned by immutable digest, not by tag: a tag (even a version tag) can be
# re-pointed at new content upstream, which would silently execute unreviewed
# third-party code in this runner on the next scheduled run.
# Digest below == returntocorp/semgrep:1.166.0 (multi-arch index, pushed 2026-06-11).
# To refresh the pin (and update this comment):
# docker manifest inspect returntocorp/semgrep:<version> -v | grep -m1 Digest
image: returntocorp/semgrep@sha256:c180f0c93a17b420c0af5006214a29d3c747c5459c732b740191adf657dd0068

@07souravkunda 07souravkunda Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[blocking] The completion comment posted on the parent ticket skips the plain-English summary and is free-form prose instead of the standard structure.

(Anchored here because this line is the change the comment describes — the finding itself is tracker-side, not in this diff.)

Every factual claim in it checks out. I re-derived all three CVSS scores from their own vectors (8.7 / 8.0 / 6.5 against the tickets' 8.4 / 7.5 / 5.9), so the flagged mismatch is real and not a v3.1-vs-v4.0 artifact of the same advisory; the digest confirms independently against registry-1.docker.io; and the dead-Travis claim holds (zero status contexts on master's head, composer install present at .travis.yml:9 but nothing runs it). The content is accurate — the shape is the problem.

Evidence — the posted comment opens straight into ### Vulnerability assessment with a paragraph of CVSS vectors. The required shape is a 🔎 Summary heading first (one TL;DR line a non-security reader understands, then Attacker / Path / Impact / Fix bullets), followed by the bold-label bullet sections — Assessment, Fix, Testing, Decision. What is posted has no Summary at all, no Decision heading, and runs as narrative paragraphs rather than labelled bullets.

Concretely, the reader gets no one-liner telling them: anyone who could push to the returntocorp Docker Hub account could have re-pointed the 1.166.0 tag, and the next 6am cron would have run their code inside a runner holding security-events: write. That sentence is the whole ticket for a non-security triager, and it is the one thing missing.

Fix — edit the comment in place (same commentId), prepending the 🔎 Summary section and reflowing the existing sections into the bold-label bullet skeleton. The locsec-fix-done JSON marker block stays as-is; nothing under it needs to change.

@07souravkunda 07souravkunda Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed — edited in place on the same commentId (2283185), so the history stays single-threaded rather than a correction stacked underneath.

The comment now opens with 🔎 Summary: a one-line TL;DR followed by Attacker / Path / Impact / Fix bullets, then Assessment / Fix / Testing / Residuals / Decision as bold-label bullet lists. I used your sentence more or less verbatim as the TL;DR — anyone who could push to the returntocorp Docker Hub account could have re-pointed the 1.166.0 tag, and the next 6am cron would have run their code inside a runner holding security-events: write — because it is a better opening line than what I had. Attacker is stated as a different principal (an outside publisher, so a real trust boundary), which was the other thing a triager couldn't tell from the old prose.

One deviation from your instruction, flagged rather than done quietly: I did not keep the marker block as-is — I shrank it. It was ~4 KB of JSON that re-encoded the whole assessment/fix/testing prose. The skill's § Marker section makes that a hard size guard, not a preference: only disposition, operator and pr_url are machine-read, and a ~15 KB ADF body has tripped the 300s addCommentToJiraIssue timeout before. Since the reflow into Summary + bullets adds visible text, keeping the fat marker pushed toward that ceiling for no benefit, so it is now the minimal {"schema":"locsec/v1","disposition":...,"operator":...,"pr_url":...}. Nothing machine-read was dropped, and every fact that was only in the JSON is now in a visible section. Say the word if you'd rather I restore the long form.

While reflowing I also folded in what changed this round — the Also affects bullet pointing at LOC-6927, and a Residuals section covering both the composer.lock constraint and the Dependabot-can't-bump-container-digests limitation — so the parent comment matches the tracker state after the sibling routing above.

And thank you for re-deriving all three CVSS scores independently rather than taking mine; that mismatch claim is load-bearing for the ticket and it's worth more having been checked twice.

# Skip any PR created by dependabot to avoid permission issues:
if: (github.actor != 'dependabot[bot]')

Expand All @@ -37,11 +46,46 @@ jobs:
# Run the "semgrep ci" command on the command line of the docker image.
- run: semgrep ci --sarif --output=semgrep.sarif
env:
# Add the rules that Semgrep uses by setting the SEMGREP_RULES environment variable.
# Add the rules that Semgrep uses by setting the SEMGREP_RULES environment variable.
SEMGREP_RULES: p/default # more at semgrep.dev/explore

# Hand the SARIF to the upload job as an artifact. `semgrep ci` exits non-zero when
# it has blocking findings, so this must run even on failure.
- name: Upload SARIF as a workflow artifact
uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2
with:
name: semgrep-sarif
path: semgrep.sarif
if-no-files-found: error
if: always()

# Separate job so that `security-events: write` is never held by the job running the
# third-party Semgrep image. This job runs no third-party code beyond first-party
# GitHub actions, all digest-pinned.
upload-sarif:
name: Upload SARIF to GitHub Advanced Security Dashboard
needs: semgrep
if: always() && (github.actor != 'dependabot[bot]')
runs-on: ubuntu-latest
permissions:
contents: read
security-events: write # for github/codeql-action/upload-sarif to upload SARIF results

steps:
# Tolerate a missing artifact. If the semgrep job died BEFORE `semgrep ci` ran — a
# container-pull failure, or checkout failing — no SARIF was ever written, and this
# job should not add a second red pointing at artifact download when the real cause
# is upstream. The container job's own failure already tells that story.
# (`semgrep ci` exiting 1 on blocking findings is the normal case: the SARIF exists,
# the artifact uploads, and the upload below runs as usual.)
- name: Download SARIF artifact

@07souravkunda 07souravkunda Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[nit] When the semgrep job dies before writing the SARIF, this job now turns red too, with a misleading reason.

The interesting failure mode — semgrep ci exits 1 on blocking findings — is handled correctly, and run 31599734585 proves it: the container job failed and the SARIF still reached the dashboard through this job. That is the case that mattered and it works.

The uncovered case is a failure upstream of semgrep ci: a container-pull failure (registry outage, or the pinned digest going away), or actions/checkout failing. Then no step runs, Upload SARIF as a workflow artifact never fires, and if-no-files-found: error is moot because there is no artifact at all. upload-sarif still starts (if: always()), Download SARIF artifact errors on a missing artifact, and the run shows two red jobs where the pre-split workflow showed one. The second one points at artifact download rather than at the actual cause.

Not a correctness problem — nothing is silently lost, and the dashboard behaviour is unchanged. Purely about not sending someone debugging a registry outage to a download step.

If you want it tighter, either gate the job on the artifact-producing step having run, or let the download tolerate absence:

      - name: Download SARIF artifact
        uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4.3.0
        with:
          name: semgrep-sarif
        continue-on-error: true

with the upload step then guarded on the file existing. Equally fine to leave as-is and let the container job's own red tell the story.

@07souravkunda 07souravkunda Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Taken — 233e0b0. You offered "equally fine to leave as-is", but the case you describe is real and I'd rather not ship a workflow that points a debugger at the wrong step during a registry outage.

Went with your second suggestion, both halves of it:

      - name: Download SARIF artifact
        uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4.3.0
        with:
          name: semgrep-sarif
        continue-on-error: true

      - name: Upload SARIF file for GitHub Advanced Security Dashboard
        uses: github/codeql-action/upload-sarif@6c089f53dd51dc3fc7e599c3cb5356453a52ca9e # v2.20.0
        with:
          sarif_file: semgrep.sarif
        if: hashFiles('semgrep.sarif') != ''

The guard on the upload step is what makes continue-on-error safe rather than just quieter — without it the upload would run against a nonexistent file and fail anyway, one step later. hashFiles() returns an empty string for a missing path, and this job does no checkout, so the file exists only if the download actually produced it.

I preferred this over gating the job on a needs.semgrep.outputs.* flag: job outputs from a failed job are propagated, so it would work, but it means threading a step output through a job output to express something the file's presence already states directly.

Net effect is what you asked for — an upstream failure now shows exactly one red job, the container job, which is the one that can explain itself. The case that mattered is unchanged: semgrep ci exiting 1 on blocking findings still writes the SARIF, uploads it, and reaches the dashboard, which is what run 31599734585 demonstrated.

No new CI evidence for the new path, and I'd rather say so than imply otherwise: forcing a container-pull failure would mean pushing a deliberately bogus digest, and I didn't think a broken commit in the PR history was worth it. The reasoning above is the argument; the normal path stays covered by the runs already on the PR.

uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4.3.0
with:
name: semgrep-sarif
continue-on-error: true

- name: Upload SARIF file for GitHub Advanced Security Dashboard
uses: github/codeql-action/upload-sarif@6c089f53dd51dc3fc7e599c3cb5356453a52ca9e # v2.20.0
with:
sarif_file: semgrep.sarif
if: always()
if: hashFiles('semgrep.sarif') != ''

@07souravkunda 07souravkunda Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[nit] This does what I asked for, and it works — but it's worth naming the trade it makes, since I'm the one who suggested it.

Verified on the new head: run 31607926461 — Download SARIF artifact success, this step success (not skipped), and Semgrep OSS shows up as a check on 233e0b04, so the SARIF genuinely reached the dashboard. hashFiles() resolves against GITHUB_WORKSPACE and download-artifact lands there by default, so the path lines up. The implicit success() that GitHub ANDs into any non-status if also stays true, because continue-on-error leaves the job status untouched.

The trade: continue-on-error + a file-existence gate can't tell "no SARIF was ever produced" (benign — the container job already went red for the real reason) apart from "the SARIF existed and was uploaded, but the download failed" (a transient artifact-service or network failure). The first is what we wanted to stop being noisy. The second used to be a red job; now it's a green run with nothing delivered to the dashboard, and no signal anywhere.

I've reasoned that path rather than exercised it — the PR runs only cover the happy path, and I wouldn't ask you to force an artifact-download failure to prove it.

Low stakes either way: the next push or the 6am cron re-uploads, so a missed upload self-heals within a day, and code scanning is not the enforcement gate here. Leaving it as-is is a defensible call. If you'd rather keep the loud signal for the case that deserves it, gating on the producing step is the discriminator — e.g. give the semgrep ci step an id and pass its outcome through a job output, then let a genuine download failure stay red when that step actually ran.

Loading