Skip to content

ci: upload Snyk Code SARIF to GitHub code scanning - #1159

Merged
dangrondahl merged 2 commits into
mainfrom
ci/snyk-sarif-code-scanning
Sep 10, 2026
Merged

dangrondahl merged 2 commits into
mainfrom
ci/snyk-sarif-code-scanning

Conversation

@dangrondahl

@dangrondahl dangrondahl commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Snyk Code already produces a SARIF report, but it was only attested to Kosli — the findings never showed up under Security → Code scanning. This uploads the same report there too, so Snyk results get GitHub's alert triage, dismissal tracking and history alongside the Kosli attestation.

Changes

  • security-events: write on the snyk-code-test job. The repo's default GITHUB_TOKEN permission is write, so main.yml and release.yml already grant it — no call-site permission changes.
  • SARIF output renamed snyk-code.jsonsnyk-code.sarif (and the sibling snyk-dependency.json.sarif, so the two agree on extension for the same format). upload-sarif expects that extension; kosli attest snyk is unaffected — internal/snyk/snyk.go:50 validates only the $schema field (Snyk's oasis-tcs URL is on its allowlist) and never looks at the filename. Nothing else in the repo referenced the old names.
  • New github/codeql-action/upload-sarif step, SHA-pinned to v4.37.9, category: snyk-code, continue-on-error: true.
  • New upload_sarif input on test.yml, default false.

Who uploads, and why it is an input

if: ${{ (success() || failure()) && inputs.upload_sarif && hashFiles('snyk-code.sarif') != '' }}
continue-on-error: true
  • success() || failure() mirrors the existing Kosli attest step, since snyk code test exits non-zero when it finds issues.
  • hashFiles(...) guards the case where snyk (or an earlier step) died before writing the report — otherwise upload-sarif aborts with Path does not exist and buries the real error.
  • inputs.upload_sarif keeps test.yml agnostic about how its three callers are wired, matching how run_snyk / report_to_kosli / checkout_ref already work. main.yml passes ${{ github.event_name == 'push' }}; release.yml and daily-cli-tests.yml leave it at the default.

The reason only push events upload: main.yml also runs on pull_request_target (the dependabot path), where github.ref is the base branch while checkout_ref is the PR head SHA — an upload there would file PR findings against main's baseline. Keeping that decision at the call site puts it next to the on: block it depends on, and a future caller defaults to not uploading rather than uploading under a mis-attributed ref.

continue-on-error because docker needs test in main.yml, and goreleaser / binary-provenance chain off it in release.yml. The SARIF 10 MB / 25 000-result limits and API 5xx are outside this repo's control, and ancillary reporting should not block a publish — the Kosli attestation remains the gate. A failed upload still shows as failed-but-ignored in the run UI.

Net effect: main gets the baseline, feature branches get their own alerts, dependabot PRs still attest to Kosli but do not upload.

Worth knowing

  • The Security tab will show Snyk's view minus the .snyk exclude: global entries (internal/azure/azure_apps.go, cmd/kosli/root.go, and the two test fixtures), so a green tab is not the same as full coverage of the tree.
  • No upload for snyk-dependency-test — Dependabot already covers dependency CVEs.
  • This PR's own run will not upload: it arrives as pull_request_target, which pre-build skips for non-dependabot actors. The first upload, and the baseline, land on the merge push to main.

Checklist

  • Docs are autogenerated from CLI help — n/a, CI-only change
  • Helm chart (charts/k8s-reporter/) updated, if needed — n/a
  • Terraform provider and related changes updated, if needed — n/a

Snyk Code already produces a SARIF report, but it was only attested to
Kosli, so the findings never appeared in the repository Security tab.
Upload the same report to GitHub code scanning as well.

- Grant security-events: write to the snyk-code-test job. The repo's
  default GITHUB_TOKEN permission is write, so main.yml and release.yml
  already allow it; no call-site changes needed.
- Rename the output to snyk-code.sarif. upload-sarif expects that
  extension; kosli attest snyk is unaffected, as internal/snyk/snyk.go
  validates only the $schema field and ignores the filename.
- Upload on branch pushes only. On pull_request_target (the dependabot
  path) github.ref is the base branch while the checkout is the PR head,
  so uploading there would attribute PR findings to main. Tag pushes
  from release.yml are skipped as well.
@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @dangrondahl's task in 2m 39s —— View job


Review — ci: upload Snyk Code SARIF to GitHub code scanning

  • Gather context (diff vs origin/main, all three callers of test.yml)
  • Static checks (go build ./... — clean; no Go changes in this PR)
  • Verify the claims in the workflow comments against the actual on: blocks
  • Review

Verdict: LGTM, no blockers. Both points from the previous round are addressed, and the input-based gate is a genuine improvement over the github.event_name check inside the callee.


Verified

I checked the load-bearing claims rather than taking the comments at face value:

Claim Result
"Tags never reach this workflow (see branches-ignore)" — main.yml:95 main.yml:3-6 filters push with only branches-ignore, and a push filtered on branches doesn't fire for tag pushes. Correct.
release.yml / daily-cli-tests.yml leave upload_sarif unset release.yml:93, daily-cli-tests.yml:31 — both default to false. daily also sets run_snyk: false, so the job is skipped entirely there.
.sarif rename is safe for kosli attest snyk internal/snyk/snyk.go:50 matches only on the $schema prefix; the filename is never inspected.
No stale snyk-code.json references ✅ Nothing left in .github/ or Makefile.
hashFiles('snyk-code.sarif') resolves correctly hashFiles is rooted at GITHUB_WORKSPACE, which is where snyk code test writes. Returns '' when snyk died before writing, so the real error stays visible.

Two subtleties that make this work but aren't obvious from the diff, both landing right:

  • continue-on-error doesn't leak into the Kosli step. A step that fails under continue-on-error gets conclusion: success (only outcome is failure), so failure() in Report Snyk Code to Kosli is not tripped by an upload failure. The attest step's behaviour is genuinely unchanged.
  • upload_sarif: ${{ github.event_name == 'push' }} evaluates to a real boolean, which is what a type: boolean workflow_call input needs — a quoted 'true' would be rejected at dispatch. ✓
  • security-events: write introduces no new permission dependency. The job already asked for contents: write, so it already required a permissive default GITHUB_TOKEN; under a read-only default it would have failed before this PR too.

Attribution is also right: on push, main.yml passes checkout_ref = $GITHUB_SHA, which is exactly the github.sha/github.ref pair upload-sarif defaults to — no mismatch between the scanned tree and the ref the alerts are filed against.


Minor notes (none blocking)

1. Scope beyond the PR description. The dependency job's output was renamed too (test.yml:287,297, snyk-dependency.json.sarif) but the PR body only mentions snyk-code. Harmless — same $schema-only validation applies — and it's the more consistent naming. Worth a line in the description so the diff matches the story.

2. docker.yml:346 is now the odd one out, still writing snyk-docker.json. Optional follow-up for consistency.

3. snyk-dependency.sarif is produced but not uploaded. Reasonable scope line to draw, and the rename hints you may already be thinking about it — a follow-up with category: snyk-dependency would slot in cleanly next to the existing category: snyk-code.

4. Comment nit (test.yml:229-236). The soft-failure rationale names docker in main.yml and goreleaser/binary-provenance in release.yml. That's the same callee-knows-its-callers coupling the upload_sarif input just removed from the logic, now living in a comment where nothing will catch it drifting. Something like "the callers gate their publish jobs on this workflow, so ancillary reporting shouldn't block them" keeps the why without the inventory. Purely cosmetic — the hashFiles and event-gating comments are exactly the right density.

5. Silent staleness on upload skip. If snyk dies before writing the report on a main push, the guard skips the upload and code scanning keeps the previous run's alerts. That's the right trade (better than wiping the baseline on a transient failure), just worth knowing the tab can lag a broken scan.


Nice work

  • The upload_sarif input is the right shape: test.yml stays agnostic about its three callers, and the dependabot/tag reasoning now lives next to the on: block it actually depends on. A future workflow_dispatch caller fails loudly at the call site instead of quietly not uploading.
  • Layering hashFiles after inputs.upload_sarif in the if reads well — cheap boolean first, filesystem probe only when it matters.
  • category: snyk-code from the start keeps the door open for a second uploader without the two clobbering each other's analyses.
  • Calling out the .snyk exclude: global entries in the PR body is the kind of thing that gets discovered six months later by someone trusting a green Security tab. Good that it's written down.
    · branch ci/snyk-sarif-code-scanning

@github-advanced-security

Copy link
Copy Markdown

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:

  • The 'Security' tab will display more code scanning analysis results (e.g., for the default branch).
  • Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results.
  • You will be able to see the analysis results for the pull request's branch on this overview once the scans have completed and the checks have passed.

For more information about GitHub Code Scanning, check out the documentation.

Comment thread .github/workflows/test.yml
Comment thread .github/workflows/test.yml Outdated
Review follow-ups on the Snyk Code SARIF upload:

- Move the branch/event decision out of the reusable workflow and into a
  new upload_sarif input, so test.yml stays agnostic about how its three
  callers are wired. main.yml passes github.event_name == 'push';
  release.yml and daily-cli-tests.yml leave it at the default of false.
  Tags never reach main.yml, so the refs/tags check is gone with it.
- Guard on hashFiles('snyk-code.sarif') != '', so a snyk run that dies
  before writing the report is not followed by a confusing
  "Path does not exist" from upload-sarif.
- continue-on-error: true. docker in main.yml and goreleaser and
  binary-provenance in release.yml all need this workflow, and the SARIF
  size limits or a GitHub 5xx are outside this repo's control; ancillary
  reporting should not block a publish. The Kosli attestation remains the
  gate.
- Name the sibling report snyk-dependency.sarif to match. No upload is
  added for it: Dependabot already covers that ground.
@dangrondahl
dangrondahl enabled auto-merge (squash) September 10, 2026 12:41
@dangrondahl
dangrondahl merged commit 687a8de into main Sep 10, 2026
25 checks passed
@dangrondahl
dangrondahl deleted the ci/snyk-sarif-code-scanning branch September 10, 2026 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants