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

[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.

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.

[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.

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.

[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.

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.

[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.