diff --git a/.github/actions/ci-test-notify/README.md b/.github/actions/ci-test-notify/README.md index 0ea6f139..f58f5c31 100644 --- a/.github/actions/ci-test-notify/README.md +++ b/.github/actions/ci-test-notify/README.md @@ -8,17 +8,20 @@ Replaces the nightly-specific `ci-notify-nightly-tests` action with a generic in -| INPUT | TYPE | REQUIRED | DEFAULT | DESCRIPTION | -|-------------|--------|----------|---------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| details | string | false | | Markdown text appended after the build
URL (test results, versions, artifact links, etc.) | -| status | string | true | | Run status, typically `needs..result` or `job.status`.
`success` and `failure` notify; `cancelled` and
`skipped` are treated as no-ops and
send nothing. | -| test-name | string | true | | Test suite name for the header
(e.g. "E2E Ginkgo Nightly Tests"). Keep under ~130 chars —
Slack header blocks have a 150-char
limit and the status suffix takes
~15 chars. | -| webhook-url | string | true | | Slack incoming webhook URL | +| INPUT | TYPE | REQUIRED | DEFAULT | DESCRIPTION | +|-------------------|--------|----------|---------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| details | string | false | | Markdown text appended after the build
URL (test results, versions, artifact links, etc.) | +| run-link-position | string | false | `"top"` | Where the immutable workflow-run link goes,
and how it reads. `top` (default)
puts a bare `Build URL: ` line above
`details`, unchanged from before this input
existed. `bottom` puts a linked `Workflow: View workflow run`
line below `details`, so the content
leads and the link trails. Invalid
values fall back to `top`. | +| status | string | true | | Run status, typically `needs..result` or `job.status`.
`success`, `failure`, and `warning` notify; `cancelled`
and `skipped` are treated as no-ops
and send nothing. | +| test-name | string | true | | Test suite name for the header
(e.g. "E2E Ginkgo Nightly Tests"). Keep under ~130 chars —
Slack header blocks have a 150-char
limit and the status suffix takes
~15 chars. | +| webhook-url | string | true | | Slack incoming webhook URL | ## Message format +With `run-link-position: top` (the default), unchanged from before that input existed: + ``` [emoji] [test-name] [status] ───────────────────────────── @@ -29,6 +32,28 @@ Build URL: · Run # ``` +With `run-link-position: bottom`, for messages whose `details` are the point and +should be read first: + +``` +[emoji] [test-name] [status] +───────────────────────────── +
+ +Workflow: View workflow run +───────────────────────────── + · Run # +``` + +The link is not merely moved: `top` prints the bare URL after `Build URL:`, while +`bottom` renders a linked label. `top` is left exactly as it was so that switching +position is opt-in for the roughly thirty existing call sites. + +The section is capped at Slack's 3000-character limit and the header at 150. Both +are measured in characters rather than bytes, so multi-byte text is not truncated +early or cut mid-character; with `bottom`, the run link is always preserved and the +`details` are what give way. + ## Usage ### Nightly E2E tests @@ -79,7 +104,13 @@ The action only notifies on actionable outcomes. A `status` of `cancelled` or `skipped` is treated as a no-op: the action logs a notice and sends nothing. This means callers can pass `needs..result` or `job.status` straight through without a guard. A cancelled run (aborted by a human or superseded) or a -skipped job never produces a Slack alert; only `success` and `failure` do. +skipped job never produces a Slack alert. + +Everything else notifies. `success` and `failure` are the usual pair; `warning` +is for an advisory result that is worth reporting but is not a failure, such as a +CVE scan running on the default non-blocking posture. An unrecognised status also +notifies, under a `❓ Unknown ()` header, on the grounds that a status +nobody anticipated is more useful surfaced than swallowed. An empty `webhook-url` (fork PRs, where secrets are unavailable) also suppresses the notification. diff --git a/.github/actions/ci-test-notify/action.yml b/.github/actions/ci-test-notify/action.yml index 4f40dd6e..48a845d3 100644 --- a/.github/actions/ci-test-notify/action.yml +++ b/.github/actions/ci-test-notify/action.yml @@ -8,7 +8,7 @@ inputs: description: 'Test suite name for the header (e.g. "E2E Ginkgo Nightly Tests"). Keep under ~130 chars — Slack header blocks have a 150-char limit and the status suffix takes ~15 chars.' required: true status: - description: 'Run status, typically `needs..result` or `job.status`. `success` and `failure` notify; `cancelled` and `skipped` are treated as no-ops and send nothing.' + description: 'Run status, typically `needs..result` or `job.status`. `success`, `failure`, and `warning` notify; `cancelled` and `skipped` are treated as no-ops and send nothing.' required: true details: description: 'Markdown text appended after the build URL (test results, versions, artifact links, etc.)' @@ -17,6 +17,10 @@ inputs: webhook-url: description: 'Slack incoming webhook URL' required: true + run-link-position: + description: 'Where the immutable workflow-run link goes, and how it reads. `top` (default) puts a bare `Build URL: ` line above `details`, unchanged from before this input existed. `bottom` puts a linked `Workflow: View workflow run` line below `details`, so the content leads and the link trails. Invalid values fall back to `top`.' + required: false + default: 'top' runs: using: "composite" @@ -40,6 +44,7 @@ runs: RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} REPO: ${{ github.repository }} RUN_NUMBER: ${{ github.run_number }} + RUN_LINK_POSITION: ${{ inputs.run-link-position }} run: ${{ github.action_path }}/build-payload.sh - name: Send Slack notification diff --git a/.github/actions/ci-test-notify/build-payload.sh b/.github/actions/ci-test-notify/build-payload.sh index 1e1ffb78..eb40140c 100755 --- a/.github/actions/ci-test-notify/build-payload.sh +++ b/.github/actions/ci-test-notify/build-payload.sh @@ -2,12 +2,25 @@ set -euo pipefail # Required env vars: TEST_NAME, STATUS, DETAILS, PAYLOAD_FILE, RUN_URL, REPO, RUN_NUMBER +# Optional env vars: RUN_LINK_POSITION (top or bottom; defaults to top) command -v jq >/dev/null || { echo "::error::jq is required but not found"; exit 1; } +# Slack's block limits are in characters. bash's ${#var} and ${var:0:n} follow +# the locale: characters under a UTF-8 locale, bytes under POSIX. Runners are +# not guaranteed to set one, so measuring in bash truncates roughly three times +# too early on non-ASCII text and can cut a UTF-8 sequence mid-character. jq +# always counts codepoints, so measure and cut there instead. +str_len() { printf '%s' "$1" | jq -Rs 'length'; } +clip_to() { + printf '%s' "$2" | jq -Rrs --argjson n "$1" \ + 'if length > $n then .[0:($n - 3)] + "..." else . end' +} + case "$STATUS" in success) EMOJI="✅"; STATUS_TEXT="Success" ;; failure) EMOJI="❌"; STATUS_TEXT="Failed" ;; + warning) EMOJI="⚠️"; STATUS_TEXT="Warning" ;; cancelled) EMOJI="⚠️"; STATUS_TEXT="Cancelled" ;; skipped) EMOJI="⏭️"; STATUS_TEXT="Skipped" ;; *) EMOJI="❓"; STATUS_TEXT="Unknown ($STATUS)" ;; @@ -16,20 +29,58 @@ esac HEADER="${EMOJI} ${TEST_NAME} ${STATUS_TEXT}" # Slack header blocks reject >150 chars -if [[ ${#HEADER} -gt 150 ]]; then - echo "::warning::Header exceeds 150-char Slack limit (${#HEADER} chars), truncating" - HEADER="${HEADER:0:147}..." +HEADER_LEN=$(str_len "$HEADER") +if [[ $HEADER_LEN -gt 150 ]]; then + echo "::warning::Header exceeds 150-char Slack limit (${HEADER_LEN} chars), truncating" + HEADER=$(clip_to 150 "$HEADER") +fi + +# Normalise first, so the two positions are each written once and every later +# reader (the truncation branch below included) sees a value it can trust. +RUN_LINK_POSITION="${RUN_LINK_POSITION:-top}" +if [[ "$RUN_LINK_POSITION" != "top" && "$RUN_LINK_POSITION" != "bottom" ]]; then + echo "::warning::invalid RUN_LINK_POSITION '$RUN_LINK_POSITION', defaulting to top" + RUN_LINK_POSITION="top" fi -SECTION="Build URL: ${RUN_URL}" -if [[ "$DETAILS" =~ [^[:space:]] ]]; then - SECTION="$(printf '%s\n\n%s' "$SECTION" "$DETAILS")" +# The two positions render the link differently, not just in a different place: +# `top` keeps the bare `Build URL:` line every existing caller already gets, and +# `bottom` uses a linked label that reads better as a footer. Changing `top` +# would alter the message for ~30 call sites, so the difference is documented in +# the input rather than smoothed over here. +RUN_LINK="Workflow: <${RUN_URL}|View workflow run>" +if [[ "$RUN_LINK_POSITION" == "bottom" ]]; then + SECTION="$RUN_LINK" + if [[ "$DETAILS" =~ [^[:space:]] ]]; then + SECTION="$(printf '%s\n\n%s' "$DETAILS" "$SECTION")" + fi +else + SECTION="Build URL: ${RUN_URL}" + if [[ "$DETAILS" =~ [^[:space:]] ]]; then + SECTION="$(printf '%s\n\n%s' "$SECTION" "$DETAILS")" + fi fi # Slack section blocks reject >3000 chars -if [[ ${#SECTION} -gt 3000 ]]; then - echo "::warning::Section exceeds 3000-char Slack limit (${#SECTION} chars), truncating" - SECTION="${SECTION:0:2997}..." +SECTION_LEN=$(str_len "$SECTION") +if [[ $SECTION_LEN -gt 3000 ]]; then + echo "::warning::Section exceeds 3000-char Slack limit (${SECTION_LEN} chars), truncating" + if [[ "$RUN_LINK_POSITION" == "bottom" ]]; then + # Reserve the run link and the blank line above it, so truncation never + # costs the one immutable piece of the message. + DETAILS_LIMIT=$((3000 - $(str_len "$RUN_LINK") - 2)) + if [[ $DETAILS_LIMIT -lt 4 ]]; then + # A run URL long enough to leave no room for details is not reachable from + # github.server_url/run_id, but an unfloored budget here would go negative + # and a negative slice reads as "all but the last n", overshooting 3000 + # and getting the whole message rejected. Keep the link, drop the details. + SECTION=$(clip_to 3000 "$RUN_LINK") + else + SECTION="$(printf '%s\n\n%s' "$(clip_to "$DETAILS_LIMIT" "$SECTION")" "$RUN_LINK")" + fi + else + SECTION=$(clip_to 3000 "$SECTION") + fi fi jq -n \ diff --git a/.github/actions/ci-test-notify/should-notify.sh b/.github/actions/ci-test-notify/should-notify.sh index a229af83..d406726d 100755 --- a/.github/actions/ci-test-notify/should-notify.sh +++ b/.github/actions/ci-test-notify/should-notify.sh @@ -5,10 +5,10 @@ set -euo pipefail # `notify=true|false` to $GITHUB_OUTPUT for the composite action to gate on. # # Callers pass the run conclusion straight from `needs..result` or -# `job.status`, which can be success, failure, cancelled, or skipped. Only -# success and failure are actionable: a cancelled run was aborted by a human -# (or superseded), and a skipped job never executed. Neither warrants a Slack -# alert, so both are silenced here rather than in every caller. +# `job.status`, which can be success, failure, warning, cancelled, or skipped. +# Cancelled and skipped runs are silenced: a cancelled run was aborted by a +# human (or superseded), and a skipped job never executed. A warning is an +# advisory result and should notify without being labelled as a failure. # # An empty webhook (fork PRs, where secrets are unavailable) also suppresses # the notification, same as before. @@ -22,7 +22,7 @@ if [[ -z "${WEBHOOK_URL:-}" ]]; then echo "::warning::webhook-url is empty (expected on fork PRs where secrets are unavailable), skipping notification" notify=false elif [[ "${STATUS:?STATUS is required}" == "cancelled" || "$STATUS" == "skipped" ]]; then - echo "::notice::status is '$STATUS' — only success and failure notify, skipping Slack notification" + echo "::notice::status is '$STATUS' — cancelled and skipped runs do not notify, skipping Slack notification" notify=false fi diff --git a/.github/actions/ci-test-notify/test/build-payload.bats b/.github/actions/ci-test-notify/test/build-payload.bats index 8c10ae00..1523cfd2 100644 --- a/.github/actions/ci-test-notify/test/build-payload.bats +++ b/.github/actions/ci-test-notify/test/build-payload.bats @@ -14,6 +14,7 @@ setup() { export RUN_URL="https://github.com/org/repo/actions/runs/12345" export REPO="org/repo" export RUN_NUMBER="42" + export RUN_LINK_POSITION="top" } teardown() { @@ -41,6 +42,12 @@ payload_field() { [[ "$(payload_field '.text')" == *"Failed"* ]] } +@test "warning status produces an advisory header" { + STATUS="warning" run bash "$SCRIPT" + [ "$status" -eq 0 ] + [ "$(payload_field '.blocks[0].text.text')" = "⚠️ My Test Suite Warning" ] +} + @test "cancelled status produces correct emoji and text" { STATUS="cancelled" run bash "$SCRIPT" [ "$status" -eq 0 ] @@ -110,6 +117,22 @@ payload_field() { [[ "$section" == *"Line three"* ]] } +@test "bottom run link position appends the workflow link after details" { + RUN_LINK_POSITION="bottom" DETAILS="High findings: 6" run bash "$SCRIPT" + [ "$status" -eq 0 ] + + local section + section=$(payload_field '.blocks[1].text.text') + [[ "$section" == "High findings: 6"$'\n\n'"Workflow: " ]] +} + +@test "invalid run link position safely falls back to the top" { + RUN_LINK_POSITION="hidden" run bash "$SCRIPT" + [ "$status" -eq 0 ] + [[ "$output" == *"invalid RUN_LINK_POSITION"* ]] + [ "$(payload_field '.blocks[1].text.text')" = "Build URL: https://github.com/org/repo/actions/runs/12345" ] +} + # --- Block Kit structure --- @test "payload has correct block structure" { @@ -178,6 +201,72 @@ payload_field() { [[ "$section" == *"..."* ]] } +@test "bottom run link is retained when details exceed the section limit" { + RUN_LINK_POSITION="bottom" DETAILS="$(printf 'X%.0s' {1..3000})" run bash "$SCRIPT" + [ "$status" -eq 0 ] + + local section + section=$(payload_field '.blocks[1].text.text') + [ "${#section}" -le 3000 ] + [[ "$section" == *"Workflow: " ]] +} + +# The limits Slack enforces are in characters, but bash measures in bytes under +# a POSIX locale, so an ASCII-only fixture cannot tell the two apart. These run +# the truncation path with multi-byte text, where a byte-based cut both fires +# far too early and can split a character in half. +# +# LC_ALL=C is pinned deliberately. Under a UTF-8 locale bash already counts +# characters, so these would pass whatever the script did and quietly stop +# testing anything — the same "green here, red there" trap that unsetting +# GITHUB_EVENT_NAME avoids in the cve-scan helpers. Pinning the byte locale +# reproduces the hazard wherever the suite runs. + +@test "a multi-byte section is measured in characters, not bytes" { + # 2000 three-byte bullets: 2000 characters, 6000 bytes. Under the limit by + # Slack's count, so nothing should be truncated. + LC_ALL=C DETAILS="$(printf '•%.0s' {1..2000})" run bash "$SCRIPT" + [ "$status" -eq 0 ] + + local chars + chars=$(jq -r '.blocks[1].text.text | length' "$PAYLOAD_FILE") + [ "$chars" -le 3000 ] + [[ "$(jq -r '.blocks[1].text.text' "$PAYLOAD_FILE")" != *"..."* ]] +} + +@test "truncating a multi-byte section never splits a character" { + LC_ALL=C DETAILS="$(printf '•%.0s' {1..4000})" run bash "$SCRIPT" + [ "$status" -eq 0 ] + + # U+FFFD is what a half-written UTF-8 sequence decodes to, so its absence is + # the assertion: the cut landed on a character boundary. + local section + section=$(jq -r '.blocks[1].text.text' "$PAYLOAD_FILE") + [ "$(jq -r '.blocks[1].text.text | length' "$PAYLOAD_FILE")" -le 3000 ] + [[ "$section" != *'�'* ]] +} + +@test "a bottom-positioned run link survives truncation intact" { + LC_ALL=C RUN_LINK_POSITION="bottom" DETAILS="$(printf '•%.0s' {1..4000})" run bash "$SCRIPT" + [ "$status" -eq 0 ] + + local section + section=$(jq -r '.blocks[1].text.text' "$PAYLOAD_FILE") + [ "$(jq -r '.blocks[1].text.text | length' "$PAYLOAD_FILE")" -le 3000 ] + [[ "$section" == *"Workflow: <${RUN_URL}|View workflow run>" ]] +} + +# A run URL this long is not reachable from github.server_url and github.run_id, +# but an unfloored budget would go negative here, and a negative slice length +# reads as "all but the last n" — overshooting 3000 and losing the whole message +# to a Slack rejection. +@test "an absurdly long run URL still yields a section within the limit" { + RUN_URL="https://github.com/org/repo/actions/runs/$(printf '9%.0s' {1..3200})" + RUN_LINK_POSITION="bottom" DETAILS="some findings" run bash "$SCRIPT" + [ "$status" -eq 0 ] + [ "$(jq -r '.blocks[1].text.text | length' "$PAYLOAD_FILE")" -le 3000 ] +} + @test "section is not truncated when under 3000 chars" { DETAILS="Short details" run bash "$SCRIPT" [ "$status" -eq 0 ] diff --git a/.github/actions/ci-test-notify/test/should-notify.bats b/.github/actions/ci-test-notify/test/should-notify.bats index a306861e..d345f04e 100644 --- a/.github/actions/ci-test-notify/test/should-notify.bats +++ b/.github/actions/ci-test-notify/test/should-notify.bats @@ -37,6 +37,12 @@ notify_value() { [ "$(notify_value)" = "true" ] } +@test "warning notifies" { + STATUS="warning" run bash "$SCRIPT" + [ "$status" -eq 0 ] + [ "$(notify_value)" = "true" ] +} + # --- Statuses that must stay silent (the bug this fixes) --- @test "cancelled does not notify" { diff --git a/.github/actions/cve-scan/README.md b/.github/actions/cve-scan/README.md index 39b600a9..c78eb3b8 100644 --- a/.github/actions/cve-scan/README.md +++ b/.github/actions/cve-scan/README.md @@ -61,7 +61,9 @@ Be deliberate about `severity-threshold` on the scheduled sweep. Real images usu | report-path | string | Path to a short markdown summary
— the per-severity counts and what
was scanned. Per-finding detail is in
`sarif-path`, not here. | | sarif-path | string | Path to the SARIF file the
scanner emitted, or empty if it
produced none. Upload it with `github/codeql-action/upload-sarif`
from the caller workflow to get
per-finding detail in the Security tab
— this action does not upload
it itself, to keep `security-events: write` out of
its own permission footprint. | | scanner-error | string | `true` if the scan could not
complete (timeout, registry failure, unparseable output). Distinct from finding CVEs
— never fails the job regardless
of `block-on-findings`. Also distinct from a *setup* error
(missing CLI or credential), which fails the job and leaves
this unset — config errors write
no result outputs at all. | +| slack-details | string | Bounded, scanner-neutral Slack preview for findings
at or above `severity-threshold`. | | summary | string | Slack-ready text summary, distinct from `report-path`
(the full file). Always set: a completed scan
reports the per-severity counts even when
they are all zero, and a
skipped or inconclusive run says which
it was. | +| threshold-count | string | Finding count at or above `severity-threshold`. | diff --git a/.github/actions/cve-scan/action.yml b/.github/actions/cve-scan/action.yml index 927d0227..dee55b76 100644 --- a/.github/actions/cve-scan/action.yml +++ b/.github/actions/cve-scan/action.yml @@ -78,6 +78,12 @@ outputs: low-count: description: "Low-severity finding count." value: ${{ steps.scan.outputs.low-count }} + threshold-count: + description: "Finding count at or above `severity-threshold`." + value: ${{ steps.scan.outputs.threshold-count }} + slack-details: + description: "Bounded, scanner-neutral Slack preview for findings at or above `severity-threshold`." + value: ${{ steps.scan.outputs.slack-details }} scanner-error: description: "`true` if the scan could not complete (timeout, registry failure, unparseable output). Distinct from finding CVEs — never fails the job regardless of `block-on-findings`. Also distinct from a *setup* error (missing CLI or credential), which fails the job and leaves this unset — config errors write no result outputs at all." value: ${{ steps.scan.outputs.scanner-error }} @@ -135,7 +141,7 @@ runs: NOTIFY_EVENTS: ${{ inputs.notify-events }} run: ${{ github.action_path }}/run.sh - - name: Send Slack notification + - name: Send Slack finding preview # Two gates, for two different reasons. # # notify-effective carries both the tolerant `notify` comparison and the @@ -146,50 +152,59 @@ runs: # trigger opts in by passing notify-events, rather than starting to post # by surprise. # - # Outcome gate: `outcome == 'failure'` is the third case. A config error - # (bad severity-threshold, a scanner that couldn't - # be provisioned) fails the job but sets neither `scanner-error` nor - # `has-vulnerabilities`, so without it that outcome reddened the run and - # told nobody — despite being the most actionable of the three. - # - # `outcome == 'cancelled'` is the fourth: a job-level timeout (or a - # manual cancel) kills the scan step outright, so no finish_with_* - # helper runs and neither result output gets set at all. Without this - # arm a killed scheduled scan produces no page and no Job Summary - # entry — a security scan that silently stopped scanning, which is the - # exact failure this action's error taxonomy exists to prevent. - # A cancel that lands before run.sh resolves notify-effective leaves it - # unset, so notify-events isn't applied on that path: unresolved gates - # resolve toward notifying, same as an unrecognised notify value. + # Finding alerts are advisory, not failures. Error and cancellation + # alerts stay in the following notification so their failure semantics + # and top-of-message build link remain unchanged. if: >- always() && steps.scan.outputs.notify-effective != 'false' && - (steps.scan.outputs.has-vulnerabilities == 'true' || - steps.scan.outputs.scanner-error == 'true' || + steps.scan.outputs.has-vulnerabilities == 'true' + uses: loft-sh/github-actions/.github/actions/ci-test-notify@0157c66f3034718081fd02ad01009e1ebc760129 # ci-test-notify/v1 + # Slack is not a gate. An outage, a rate limit or a rejected payload must + # not fail the caller, or a notification system becomes able to block a + # release — the same reason the registry login above is advisory. The Job + # Summary is written either way, so nothing is lost when this is skipped. + continue-on-error: true + with: + test-name: "CVE scan · ${{ steps.scan.outputs.threshold-count }} ${{ inputs.severity-threshold }} findings" + # block-on-findings turns these findings into the job's failure, so the + # alert has to say failure too. Without this a blocking run that reds the + # caller announced itself as an amber advisory and, because the error + # notification below excludes findings, that was the only alert sent. + status: ${{ steps.scan.outputs.block-effective == 'true' && 'failure' || 'warning' }} + run-link-position: bottom + details: ${{ steps.scan.outputs.slack-details }} + webhook-url: ${{ inputs.slack-webhook-url }} + + - name: Send Slack error notification + if: >- + always() && + steps.scan.outputs.notify-effective != 'false' && + steps.scan.outputs.has-vulnerabilities != 'true' && + (steps.scan.outputs.scanner-error == 'true' || steps.scan.outcome == 'failure' || steps.scan.outcome == 'cancelled') - uses: loft-sh/github-actions/.github/actions/ci-test-notify@85d7023c5749421d369f59430c7849f2d00ad694 # ci-test-notify/v1 + uses: loft-sh/github-actions/.github/actions/ci-test-notify@0157c66f3034718081fd02ad01009e1ebc760129 # ci-test-notify/v1 + # Slack is not a gate. An outage, a rate limit or a rejected payload must + # not fail the caller, or a notification system becomes able to block a + # release — the same reason the registry login above is advisory. The Job + # Summary is written either way, so nothing is lost when this is skipped. + continue-on-error: true with: test-name: "cve-scan: ${{ inputs.image-ref }}" - # Every branch of the `if:` above is a failure, so the status is a + # Every branch of this `if:` is a failure, so the status is a # literal rather than a ternary that could only ever pick one value. status: failure - # All four outcomes share that status, so the distinction has to live - # in the text: "the scanner broke", "we shipped critical CVEs", "the - # config is wrong" and "the step got killed" are very different calls - # to action for whoever is on call, and someone skimming Slack by - # colour can't tell them apart. Cancelled is checked first: on that - # outcome the step never set scanner-error or has-vulnerabilities at - # all, so it can't be distinguished from a config error any other way. - # Otherwise ordered so a blocked scan with findings reads as findings, - # not as a config error — it fails the job too. + # Scanner, configuration, and cancellation errors all share failure + # status, so their distinction lives in the details. Cancelled is + # checked first because the scan step never set its result outputs. # # The link is derived, not an input, so it can't drift from the upload: # upload-sarif defaults to the same github.ref. Needs the ref filter or # the page shows the default branch and reads as empty, and sits outside # the fence because Slack won't linkify inside one. details: | - ${{ steps.scan.outcome == 'cancelled' && 'CANCELLED — the scan step was killed (job timeout or a manual cancel) before it could finish; nothing was verified' || (steps.scan.outputs.scanner-error == 'true' && 'SCANNER ERROR — no scan was performed, nothing was verified' || (steps.scan.outputs.has-vulnerabilities == 'true' && 'FINDINGS at or above the severity threshold' || 'CONFIGURATION ERROR — cve-scan could not run. An authoring or provisioning mistake, not a scan result; see the job log.')) }} + ${{ steps.scan.outcome == 'cancelled' && 'CANCELLED — the scan step was killed (job timeout or a manual cancel) before it could finish; nothing was verified' || (steps.scan.outputs.scanner-error == 'true' && 'SCANNER ERROR — no scan was performed, nothing was verified' || 'CONFIGURATION ERROR — cve-scan could not run. An authoring or provisioning mistake, not a scan result; see the job log.') }} ``` ${{ steps.scan.outputs.summary }} ``` diff --git a/.github/actions/cve-scan/run.sh b/.github/actions/cve-scan/run.sh index 4092c188..fd17ca4d 100755 --- a/.github/actions/cve-scan/run.sh +++ b/.github/actions/cve-scan/run.sh @@ -99,6 +99,11 @@ write_output() { # caller's if: is one comparison rather than two. write_output notify-effective "$NOTIFY" +# Same reasoning, for the notification's status rather than its gate: the +# tolerant spelling is resolved here, so action.yml does not have to repeat +# the comparison in YAML to tell an amber advisory from a red failure. +write_output block-effective "$BLOCK_ON_FINDINGS" + # Every outcome leaves a Job Summary entry, including the ones with no report. # A skipped or inconclusive run is the outcome most in need of a human # noticing, and it is otherwise visible only in Annotations. @@ -274,6 +279,7 @@ fi PROCESS_EXIT=0 FINDINGS_JSON="$FINDINGS_JSON" \ REPORT_PATH="${WORKDIR}/report.md" \ + BLOCK_ON_FINDINGS="$BLOCK_ON_FINDINGS" \ "${ACTION_PATH}/src/process-findings.sh" || PROCESS_EXIT=$? if [ "$PROCESS_EXIT" -ne 0 ]; then diff --git a/.github/actions/cve-scan/src/process-findings.sh b/.github/actions/cve-scan/src/process-findings.sh index 35fb77cd..781a88ce 100755 --- a/.github/actions/cve-scan/src/process-findings.sh +++ b/.github/actions/cve-scan/src/process-findings.sh @@ -23,6 +23,7 @@ set -euo pipefail : "${IMAGE_REF:?IMAGE_REF is required}" SEVERITY_THRESHOLD="${SEVERITY_THRESHOLD:-high}" +BLOCK_ON_FINDINGS="${BLOCK_ON_FINDINGS:-false}" # The event name, not a caller label: the image ref already says whether this # is a head sweep or a release scan, and a hand-typed label can contradict it. TRIGGER_CONTEXT="${TRIGGER_CONTEXT:-${GITHUB_EVENT_NAME:-unspecified trigger}}" @@ -73,6 +74,111 @@ COUNTS_TSV=$(jq -r --arg t "$SEVERITY_THRESHOLD" --argjson rank "$SEVERITY_RANK" read -r CRITICAL_COUNT HIGH_COUNT MEDIUM_COUNT LOW_COUNT \ FINDING_COUNT HAS_VULNERABILITIES <<<"$COUNTS_TSV" +# Keep the Slack preview in the adapter-neutral layer. Every scanner emits the +# canonical finding shape above, so changing scanner never changes what a +# notification consumer receives. +THRESHOLD_COUNT=$(jq -r --arg t "$SEVERITY_THRESHOLD" --argjson rank "$SEVERITY_RANK" \ + '[.findings[] | select(($rank[.severity] // 0) >= $rank[$t])] | length' "$FINDINGS_JSON") + +# Scanner strings become mrkdwn text in ci-test-notify. Strip controls, encode +# Slack's entity delimiters, and bound each field before it reaches a webhook. +SLACK_IMAGE=$(jq -nr --arg value "$IMAGE_REF" ' + $value + | gsub("[[:cntrl:]]"; " ") + | gsub("&"; "&") | gsub("<"; "<") | gsub(">"; ">") + | if length > 200 then .[0:197] + "..." else . end +') + +PREVIEW_GROUPS=$(jq -r --arg t "$SEVERITY_THRESHOLD" --argjson rank "$SEVERITY_RANK" ' + def clean: + tostring + | gsub("[[:cntrl:]]"; " ") + | gsub("&"; "&") | gsub("<"; "<") | gsub(">"; ">"); + def clip($limit): + if length > $limit then .[0:($limit - 3)] + "..." else . end; + [ .findings[] + | select(($rank[.severity] // 0) >= $rank[$t]) + | { + package: ((.package // "unknown package") | clean | clip(160)), + title: ((.title // .id // "unnamed vulnerability") | clean | clip(150)) + } + ] + | sort_by(.package) | group_by(.package) + # Most-affected package first. Sorting by name and then capping at three + # drops the worst package whenever two others happen to sort earlier, which + # is the opposite of what a bounded preview is for. Name breaks the tie so + # the order stays stable between runs. + | sort_by(-length, .[0].package) | .[:3] + | map( + . as $group + | ($group | map(.title) | sort | group_by(.) + # Most-repeated title first, so the .[:4] cap below keeps the titles + # that dominate the group rather than the alphabetically first ones. + | sort_by(-length, .[0]) + | map(if length == 1 then .[0] else "\(.[0]) (\(length))" end) + | .[:4] | join(", ")) as $titles + | "• \($group[0].package) · \($group | length) \(if ($group | length) == 1 then "finding" else "findings" end)\n \($titles)" + ) + | join("\n\n") +' "$FINDINGS_JSON") + +BELOW_THRESHOLD=() +add_below_threshold() { + local count="$1" severity="$2" + if [ "$count" -gt 0 ]; then + if [ "$count" -eq 1 ]; then + BELOW_THRESHOLD+=("${count} ${severity} finding") + else + BELOW_THRESHOLD+=("${count} ${severity} findings") + fi + fi +} +case "$SEVERITY_THRESHOLD" in + critical) + add_below_threshold "$HIGH_COUNT" high + add_below_threshold "$MEDIUM_COUNT" medium + add_below_threshold "$LOW_COUNT" low + ;; + high) + add_below_threshold "$MEDIUM_COUNT" medium + add_below_threshold "$LOW_COUNT" low + ;; + medium) add_below_threshold "$LOW_COUNT" low ;; +esac + +# The same pair run.sh gates on. Blocking alone does not fail a job: it takes +# blocking *and* something at or above the threshold, so keying the line on the +# input by itself would promise a failure that never came. +if [ "$BLOCK_ON_FINDINGS" != "true" ]; then + BLOCKING_CONTEXT="Advisory only — release was not blocked." +elif [ "$HAS_VULNERABILITIES" = "true" ]; then + BLOCKING_CONTEXT="Blocking enabled — these findings fail the job." +else + BLOCKING_CONTEXT="Blocking enabled — nothing at or above the threshold, so the job is not blocked." +fi + +# The heading is gated on the count rather than on action.yml's step `if:`. +# slack-details is a declared output, so a caller can read it on a scan with +# nothing at or above the threshold, where an unconditional heading promised a +# list and then showed none. +SLACK_DETAILS=$(printf 'Image: %s\nThreshold: %s · %s' \ + "$SLACK_IMAGE" "$SEVERITY_THRESHOLD" "$BLOCKING_CONTEXT") +if [ "$THRESHOLD_COUNT" -gt 0 ]; then + SLACK_DETAILS="${SLACK_DETAILS}"$'\n\n'"Findings at or above threshold"$'\n\n'"${PREVIEW_GROUPS}" +fi +if [ "${#BELOW_THRESHOLD[@]}" -gt 0 ]; then + # `${array[*]}` joins on IFS's *first character* only, so IFS=', ' drops the + # space and runs two counts together as "1 medium finding,1 low finding". + printf -v BELOW_THRESHOLD_TEXT '%s, ' "${BELOW_THRESHOLD[@]}" + SLACK_DETAILS="${SLACK_DETAILS}"$'\n\n'"Also detected: ${BELOW_THRESHOLD_TEXT%, } below the notification threshold." +fi + +write_multiline_output() { + local key="$1" value="$2" delimiter + delimiter="cve-scan-${key}-$(od -An -N16 -tx1 /dev/urandom | tr -d '[:space:]')" + printf '%s<<%s\n%s\n%s\n' "$key" "$delimiter" "$value" "$delimiter" >> "$GITHUB_OUTPUT" +} + # --- Markdown report --------------------------------------------------------- { echo "# CVE Scan — ${IMAGE_REF} — ${TODAY}" @@ -105,6 +211,8 @@ read -r CRITICAL_COUNT HIGH_COUNT MEDIUM_COUNT LOW_COUNT \ echo "high-count=${HIGH_COUNT}" echo "medium-count=${MEDIUM_COUNT}" echo "low-count=${LOW_COUNT}" + echo "threshold-count=${THRESHOLD_COUNT}" echo "report-path=${REPORT_PATH}" echo "summary=cve-scan — \`${IMAGE_REF}\` (${TRIGGER_CONTEXT}): critical=${CRITICAL_COUNT} high=${HIGH_COUNT} medium=${MEDIUM_COUNT} low=${LOW_COUNT}" } >> "$GITHUB_OUTPUT" +write_multiline_output slack-details "$SLACK_DETAILS" diff --git a/.github/actions/cve-scan/test/helpers.bash b/.github/actions/cve-scan/test/helpers.bash index dfcad7d5..6034990a 100644 --- a/.github/actions/cve-scan/test/helpers.bash +++ b/.github/actions/cve-scan/test/helpers.bash @@ -10,6 +10,24 @@ grab_output() { grep -E "^$1=" "$GITHUB_OUTPUT" | tail -n1 | cut -d= -f2- } +# grab_multiline_output — reads a GitHub Actions heredoc output. +grab_multiline_output() { + local key="$1" line delimiter="" found=false output="" + while IFS= read -r line; do + if [ "$found" = false ]; then + if [[ "$line" == "$key"'<<'* ]]; then + delimiter="${line#*<<}" + found=true + fi + elif [ "$line" = "$delimiter" ]; then + break + else + output+="${output:+$'\n'}${line}" + fi + done < "$GITHUB_OUTPUT" + printf '%s\n' "$output" +} + # The temp dir, the GITHUB_OUTPUT file and the image ref every suite needs. setup_tmp_env() { # Actions sets GITHUB_EVENT_NAME on every runner, and run.sh reads it for the diff --git a/.github/actions/cve-scan/test/notification_pins.bats b/.github/actions/cve-scan/test/notification_pins.bats new file mode 100644 index 00000000..bb88121f --- /dev/null +++ b/.github/actions/cve-scan/test/notification_pins.bats @@ -0,0 +1,11 @@ +#!/usr/bin/env bats + +MANIFEST="$BATS_TEST_DIRNAME/../action.yml" +CHARACTER_SAFE_NOTIFIER="0157c66f3034718081fd02ad01009e1ebc760129" + +@test "both Slack notifications use the character-safe notifier revision" { + expected="uses: loft-sh/github-actions/.github/actions/ci-test-notify@${CHARACTER_SAFE_NOTIFIER} # ci-test-notify/v1" + count="$(grep -Fc "$expected" "$MANIFEST" || true)" + + [ "$count" -eq 2 ] +} diff --git a/.github/actions/cve-scan/test/process_findings.bats b/.github/actions/cve-scan/test/process_findings.bats index 3e87edf0..60db349f 100644 --- a/.github/actions/cve-scan/test/process_findings.bats +++ b/.github/actions/cve-scan/test/process_findings.bats @@ -205,6 +205,134 @@ JSON [[ "$summary" == *"low=4"* ]] } +@test "writes a bounded, grouped Slack preview for threshold findings" { + write_findings <<'JSON' +{"findings":[ + {"id":"A","severity":"high","package":"github.com/example/pkg","title":"Out-of-bounds read"}, + {"id":"B","severity":"high","package":"github.com/example/pkg","title":"Out-of-bounds read"}, + {"id":"C","severity":"high","package":"github.com/example/pkg","title":"Integer <@U123> & underflow"}, + {"id":"D","severity":"high","package":"github.com/example/other","title":"Invalid array index"}, + {"id":"E","severity":"medium","package":"ignored","title":"Below threshold <@U123> & more"} +]} +JSON + SEVERITY_THRESHOLD=high run bash "$SCRIPT" + [ "$status" -eq 0 ] + [ "$(grab_output threshold-count)" = "4" ] + [ "$(grep -Ec '^slack-details<"* ]] +} + +# The existing preview test leaves exactly one severity below the threshold, +# which is the one case a broken separator still renders correctly. +@test "several below-threshold severities read as a list, not one run-on count" { + write_findings <<'JSON' +{"findings":[ + {"id":"A","severity":"critical","package":"stdlib","title":"Boom"}, + {"id":"B","severity":"high","package":"stdlib","title":"Bang"}, + {"id":"C","severity":"medium","package":"stdlib","title":"Meh"}, + {"id":"D","severity":"low","package":"stdlib","title":"Tiny"}, + {"id":"E","severity":"low","package":"stdlib","title":"Tiny again"} +]} +JSON + SEVERITY_THRESHOLD=critical run bash "$SCRIPT" + [ "$status" -eq 0 ] + + local preview + preview=$(grab_multiline_output slack-details) + [[ "$preview" == *"Also detected: 1 high finding, 1 medium finding, 2 low findings below the notification threshold."* ]] +} + +# The blocking line is the alert's only statement of whether these findings +# actually fail the job, and nothing asserted it: swapping the two strings kept +# the whole suite green. + +@test "an advisory scan says the release was not blocked" { + write_findings <<'JSON' +{"findings":[{"id":"A","severity":"high","package":"stdlib","title":"Boom"}]} +JSON + SEVERITY_THRESHOLD=high BLOCK_ON_FINDINGS=false run bash "$SCRIPT" + [ "$status" -eq 0 ] + + local preview + preview=$(grab_multiline_output slack-details) + [[ "$preview" == *"Advisory only — release was not blocked."* ]] +} + +@test "a blocking scan with findings says they fail the job" { + write_findings <<'JSON' +{"findings":[{"id":"A","severity":"high","package":"stdlib","title":"Boom"}]} +JSON + SEVERITY_THRESHOLD=high BLOCK_ON_FINDINGS=true run bash "$SCRIPT" + [ "$status" -eq 0 ] + + local preview + preview=$(grab_multiline_output slack-details) + [[ "$preview" == *"Blocking enabled — these findings fail the job."* ]] +} + +# Blocking alone does not fail a job; it takes blocking and something at or above +# the threshold. Keyed on the input by itself, this promised a failure that the +# gate in run.sh was never going to deliver. +@test "a blocking scan with nothing at the threshold does not promise a failure" { + write_findings <<'JSON' +{"findings":[{"id":"A","severity":"medium","package":"stdlib","title":"Meh"}]} +JSON + SEVERITY_THRESHOLD=high BLOCK_ON_FINDINGS=true run bash "$SCRIPT" + [ "$status" -eq 0 ] + + local preview + preview=$(grab_multiline_output slack-details) + [[ "$preview" == *"Blocking enabled — nothing at or above the threshold, so the job is not blocked."* ]] + [[ "$preview" != *"these findings fail the job"* ]] +} + +# The cap exists to bound the message, not to pick winners by name. Three +# single-finding packages sort ahead of the one that actually matters here. +@test "the package cap keeps the most-affected package, not the alphabetically first" { + write_findings <<'JSON' +{"findings":[ + {"id":"1","severity":"high","package":"aaa","title":"One"}, + {"id":"2","severity":"high","package":"bbb","title":"Two"}, + {"id":"3","severity":"high","package":"ccc","title":"Three"}, + {"id":"4","severity":"high","package":"zzz","title":"Worst A"}, + {"id":"5","severity":"high","package":"zzz","title":"Worst B"}, + {"id":"6","severity":"high","package":"zzz","title":"Worst C"}, + {"id":"7","severity":"high","package":"zzz","title":"Worst D"}, + {"id":"8","severity":"high","package":"zzz","title":"Worst E"} +]} +JSON + SEVERITY_THRESHOLD=high run bash "$SCRIPT" + [ "$status" -eq 0 ] + + local preview + preview=$(grab_multiline_output slack-details) + [[ "$preview" == *"zzz · 5 findings"* ]] +} + +# slack-details is a declared output, so a caller can read it on a scan that has +# nothing at or above the threshold. The heading used to appear there regardless, +# announcing a list and then showing none. +@test "a scan with nothing at or above the threshold omits the findings heading" { + write_findings <<'JSON' +{"findings":[{"id":"A","severity":"medium","package":"stdlib","title":"Meh"}]} +JSON + SEVERITY_THRESHOLD=high run bash "$SCRIPT" + [ "$status" -eq 0 ] + + local preview + preview=$(grab_multiline_output slack-details) + [[ "$preview" != *"Findings at or above threshold"* ]] + [[ "$preview" == *"Also detected: 1 medium finding below the notification threshold."* ]] +} + @test "the report names a digest ref verbatim rather than mangling it" { IMAGE_REF="ghcr.io/loft-sh/vcluster-pro@sha256:abcdef1234567890" run bash "$SCRIPT" [ "$status" -eq 0 ] diff --git a/.github/actions/cve-scan/test/run.bats b/.github/actions/cve-scan/test/run.bats index 8c0d286a..50283acf 100644 --- a/.github/actions/cve-scan/test/run.bats +++ b/.github/actions/cve-scan/test/run.bats @@ -547,6 +547,44 @@ EOF # whole point of the fix is that a caller's Slack if: can trust this output # even when the scan itself errored, since that's exactly when a page matters. +# action.yml picks the alert's status off block-effective. An absent output makes +# `== 'true'` quietly false, which reverts a blocking run to an amber advisory +# with nothing going red, so the output's existence is asserted rather than +# assumed — the same coverage notify-effective already carries. + +@test "block-effective carries every recognised truthy spelling" { + for v in true TRUE True " true " yes YES 1 on ON; do + BLOCK_ON_FINDINGS="$v" run bash "$SCRIPT" + [ "$(grab_output block-effective)" = "true" ] || { + echo "block-on-findings='$v' did not resolve to block-effective=true" + return 1 + } + done +} + +@test "block-effective carries every recognised falsey spelling" { + for v in false FALSE False " false " no NO 0 off OFF; do + BLOCK_ON_FINDINGS="$v" run bash "$SCRIPT" + [ "$(grab_output block-effective)" = "false" ] || { + echo "block-on-findings='$v' did not resolve to block-effective=false" + return 1 + } + done +} + +@test "block-effective defaults to false when the caller says nothing" { + run bash "$SCRIPT" + [ "$status" -eq 0 ] + [ "$(grab_output block-effective)" = "false" ] +} + +@test "block-effective is set on a scanner-error outcome" { + echo "1" > "$ADAPTER_EXIT_FILE" + BLOCK_ON_FINDINGS=true run bash "$SCRIPT" + [ "$status" -eq 0 ] + [ "$(grab_output block-effective)" = "true" ] +} + @test "notify-effective is set on a scanner-error outcome" { echo "1" > "$ADAPTER_EXIT_FILE" NOTIFY=true run bash "$SCRIPT"