From 1b6e637efe6e32ea93a51a04564a03447f9b3a89 Mon Sep 17 00:00:00 2001 From: Alex Root-Roatch Date: Tue, 28 Jul 2026 13:04:13 -0500 Subject: [PATCH] feat: report semgrep findings as a CWE/OWASP table; honour nosemgrep The one-line-per-finding format said WHERE but not WHAT CLASS, so acting on a finding meant looking the rule up by hand. semgrep already carries CWE, OWASP category and confidence in tool.driver.rules[].properties.tags, so the report now renders: # SEVERITY FILE:LINE RULE CWE OWASP CONFIDENCE 1 warning src/clj/cleancoders/mfa.clj:218 cc-generic-catch 396,636 A10:2025 LOW 2 error src/cljs/cleancoders/contact.cljs:224 cc-cljs-innerhtml CWE-79 A05:2025 HIGH Identifiers stay in the table and a legend expands each CWE and OWASP category once, so rows remain scannable instead of ~200 columns wide. Fixes a real bug found while building the fixture: semgrep still emits a result for source annotated with a nosemgrep comment, marking it suppressions:[{kind:"inSource"}]. The reporting counted those, so cleancoders.com's two already-triaged secret findings were being reported AND blocking the build -- which makes nosemgrep look broken. Suppressed results are now excluded from the table, the legend and the exit code, and reported as a count so the suppression stays visible rather than silent. That repo's real number is 1 blocking, not 3. Rendering and the exit decision move into bin/report-sarif.sh with 20 tests in bin/test-report-sarif.sh, run against a SARIF captured from an actual consumer scan. The fixture has region.snippet stripped: semgrep embeds matched source, which for a secret-detection rule means the secret itself, and this repo is public. --- .github/workflows/security.yml | 40 +--- .github/workflows/self-test.yml | 3 + bin/report-sarif.sh | 112 +++++++++++ bin/test-report-sarif.sh | 70 +++++++ spec-fixtures/sarif/sample.sarif | 314 +++++++++++++++++++++++++++++++ 5 files changed, 504 insertions(+), 35 deletions(-) create mode 100644 bin/report-sarif.sh create mode 100644 bin/test-report-sarif.sh create mode 100644 spec-fixtures/sarif/sample.sarif diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 5cafe37..10d3973 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -351,41 +351,11 @@ jobs: fi semgrep "${args[@]}" - if [ ! -f semgrep.sarif ]; then - echo "::error::semgrep produced no SARIF; the scan did not complete" - exit 1 - fi - - # Severity lives on the rule, not the result: semgrep leaves - # results[].level null and puts it in tool.driver.rules[]. - # defaultConfiguration.level. Join them so the printout shows severity - # and so the exit decision can honour it. - jq -r '.runs[] as $r - | ($r.tool.driver.rules - | map({key: .id, value: .defaultConfiguration.level}) - | from_entries) as $lv - | $r.results[] - | " [\($lv[.ruleId] // "unknown")] \(.ruleId | split(".") | last) " - + "\(.locations[0].physicalLocation.artifactLocation.uri)" - + ":\(.locations[0].physicalLocation.region.startLine // 0)"' \ - semgrep.sarif - - # Only error-level findings fail the job. cc-path-traversal and - # cc-generic-catch are severity: WARNING on purpose — without dataflow - # they cannot be precise enough to gate a build, and the README - # promises they do not block. Counting every result would break that. - blocking=$(jq '[.runs[] as $r - | ($r.tool.driver.rules - | map({key: .id, value: .defaultConfiguration.level}) - | from_entries) as $lv - | $r.results[] | select($lv[.ruleId] == "error")] | length' \ - semgrep.sarif) - total=$(jq '[.runs[].results[]?] | length' semgrep.sarif) - echo "semgrep: ${total} finding(s), ${blocking} blocking" - if [ "$blocking" -gt 0 ]; then - echo "::error::semgrep found $blocking blocking finding(s); see above and the semgrep-sarif artifact" - exit 1 - fi + # Rendering and the exit decision live in bin/report-sarif.sh so both + # are testable: bin/test-report-sarif.sh runs them against a SARIF + # captured from a real consumer scan. The critical case it pins is that + # warning-level rules do not block. + bash .cc-security-rules/bin/report-sarif.sh semgrep.sarif - name: Upload SARIF if: always() # evidence of a FAILING scan is the evidence most worth keeping uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 (node24) diff --git a/.github/workflows/self-test.yml b/.github/workflows/self-test.yml index bf47329..22acf8d 100644 --- a/.github/workflows/self-test.yml +++ b/.github/workflows/self-test.yml @@ -58,6 +58,9 @@ jobs: # Replaces the old security-skips job, which duplicated all nine scanner # jobs to exercise two conditionals. 17 cases, milliseconds. run: bash bin/test-detect.sh + - name: Run SARIF reporter tests + # Pins the exit-code contract: warning-level rules must not block. + run: bash bin/test-report-sarif.sh - name: Run rule fixture tests run: bash bin/test-rules.sh - name: Check coverage table is current diff --git a/bin/report-sarif.sh b/bin/report-sarif.sh new file mode 100644 index 0000000..9f70675 --- /dev/null +++ b/bin/report-sarif.sh @@ -0,0 +1,112 @@ +#!/usr/bin/env bash +# Renders a semgrep SARIF file as a CI-readable table and sets the exit code. +# +# report-sarif.sh +# exit 0 no error-level findings +# exit 1 at least one error-level finding +# +# Why a table: the previous one-line-per-finding format told you WHERE but not +# WHAT CLASS, so acting on a finding meant looking the rule up by hand. Semgrep +# already carries CWE, OWASP category and confidence in +# tool.driver.rules[].properties.tags — this surfaces them. +# +# Severity lives on the rule, not the result: semgrep leaves results[].level null +# and puts it in tool.driver.rules[].defaultConfiguration.level. Everything below +# joins through that. +# +# Only error-level findings affect the exit code. cc-path-traversal, +# cc-generic-catch and cc-clojure-xml-xxe are WARNING on purpose — without +# dataflow they cannot be precise enough to gate a build, and the README promises +# they do not block. +# +# Results carrying a `suppressions` array are excluded from the table and the +# exit code. semgrep still emits a finding when source has a `nosemgrep` +# annotation, marking it suppressions:[{kind:"inSource"}]. Counting those means +# blocking a build on a finding a developer already triaged, and makes nosemgrep +# look broken. They are reported as a count so the suppression stays visible +# rather than silent. +set -euo pipefail + +SARIF="${1:?usage: report-sarif.sh }" + +command -v jq >/dev/null || { echo "jq not installed"; exit 1; } +[ -f "$SARIF" ] || { echo "::error::$SARIF not found; the scan did not complete"; exit 1; } + +total="$(jq '[.runs[].results[]? | select(.suppressions == null or (.suppressions | length) == 0)] | length' "$SARIF")" +suppressed="$(jq '[.runs[].results[]? | select(.suppressions != null and (.suppressions | length) > 0)] | length' "$SARIF")" +if [ "$total" -eq 0 ]; then + echo "semgrep: no findings${suppressed:+ (${suppressed} suppressed in source)}" + exit 0 +fi + +# One TSV row per finding. Tag shapes semgrep emits: +# "CWE-79: Improper Neutralization of Input ... ('Cross-site Scripting')" +# "OWASP-A05:2025 - Injection" (a rule may carry several editions) +# "HIGH CONFIDENCE" +# CWE and OWASP are reduced to identifiers here to keep the table narrow; the +# legend below prints the full names once each. +rows="$(jq -r ' + .runs[] as $r + | ($r.tool.driver.rules + | map({key: .id, value: .}) | from_entries) as $rules + | [$r.results[] | select(.suppressions == null or (.suppressions | length) == 0)] + | to_entries[] + | .key as $i | .value as $res + | ($rules[$res.ruleId] // {}) as $rule + | ($rule.properties.tags // []) as $tags + | ($rule.defaultConfiguration.level // "unknown") as $lvl + | ($tags | map(select(startswith("CWE-")) | split(":")[0]) | join(", ")) as $cwe + | ($tags | map(select(startswith("OWASP-")) | sub("^OWASP-";"") | split(" ")[0]) + | join(", ")) as $owasp + | ($tags | map(select(endswith(" CONFIDENCE")) | sub(" CONFIDENCE";"")) + | first // "-") as $conf + | [ ($i + 1 | tostring), + $lvl, + ($res.locations[0].physicalLocation.artifactLocation.uri + + ":" + ($res.locations[0].physicalLocation.region.startLine // 0 | tostring)), + ($res.ruleId | split(".") | last), + (if $cwe == "" then "-" else $cwe end), + (if $owasp == "" then "-" else $owasp end), + $conf ] + | @tsv' "$SARIF")" + +{ + printf '#\tSEVERITY\tFILE:LINE\tRULE\tCWE\tOWASP\tCONFIDENCE\n' + printf '%s\n' "$rows" +} | { command -v column >/dev/null && column -t -s "$(printf '\t')" || cat; } + +# Legend: expand each identifier once rather than repeating long names per row. +echo +echo "CWE:" +jq -r '.runs[] as $r + | ([$r.results[] | select(.suppressions == null or (.suppressions | length) == 0) + | .ruleId] | unique) as $shown + | [$r.tool.driver.rules[] | select(.id as $i | $shown | index($i)) + | .properties.tags[]? | select(startswith("CWE-"))] + | unique | .[] | " " + .' "$SARIF" +echo "OWASP:" +jq -r '.runs[] as $r + | ([$r.results[] | select(.suppressions == null or (.suppressions | length) == 0) + | .ruleId] | unique) as $shown + | [$r.tool.driver.rules[] | select(.id as $i | $shown | index($i)) + | .properties.tags[]? | select(startswith("OWASP-")) | sub("^OWASP-";"")] + | unique | .[] | " " + .' "$SARIF" + +blocking="$(jq '[.runs[] as $r + | ($r.tool.driver.rules + | map({key: .id, value: .defaultConfiguration.level}) + | from_entries) as $lv + | $r.results[] + | select(.suppressions == null or (.suppressions | length) == 0) + | select($lv[.ruleId] == "error")] | length' "$SARIF")" + +echo +echo "semgrep: ${total} finding(s), ${blocking} blocking (error-level)" +if [ "$suppressed" -gt 0 ]; then + echo " ${suppressed} additional finding(s) suppressed in source (nosemgrep)" +fi +if [ "$blocking" -gt 0 ]; then + echo "::error::semgrep found ${blocking} blocking finding(s); see the table above and the semgrep-sarif artifact" + exit 1 +fi +exit 0 diff --git a/bin/test-report-sarif.sh b/bin/test-report-sarif.sh new file mode 100644 index 0000000..4cdc987 --- /dev/null +++ b/bin/test-report-sarif.sh @@ -0,0 +1,70 @@ +#!/usr/bin/env bash +# Tests bin/report-sarif.sh against a real SARIF captured from a consumer run. +# +# The exit code here decides whether a build passes, so the warnings-only case is +# the one that matters: cc-path-traversal, cc-generic-catch and +# cc-clojure-xml-xxe are WARNING on purpose and the README promises they do not +# block. A regression that counted every result would quietly start failing +# builds on low-precision findings. +set -uo pipefail + +ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +REPORT="${ROOT}/bin/report-sarif.sh" +SAMPLE="${ROOT}/spec-fixtures/sarif/sample.sarif" +pass=0; fail=0 + +ok() { pass=$((pass+1)); } +bad() { fail=$((fail+1)); echo "FAIL: $1"; } +check() { if [ "$2" = "$3" ]; then ok; else bad "$1 — expected '$3', got '$2'"; fi; } + +WORK="$(mktemp -d)" +trap 'rm -rf "${WORK}"' EXIT + +# --- the real sample: 7 results, 2 nosemgrep-suppressed, 1 error-level -------- +# The suppressed pair is the important part. cleancoders.com annotated both +# secret findings with `nosemgrep`; semgrep honours that by setting +# suppressions:[{kind:"inSource"}] but still emits the result. Counting them +# blocks a build on findings a developer already accepted. +out="$(bash "${REPORT}" "${SAMPLE}" 2>&1)"; rc=$? +check "one error-level finding blocks" "$rc" "1" +check "excludes suppressed from the counts" "$(echo "$out" | grep -c '5 finding(s), 1 blocking')" "1" +check "reports the suppressed count" "$(echo "$out" | grep -c '2 additional finding(s) suppressed')" "1" +check "emits one row per unsuppressed finding" "$(echo "$out" | grep -cE '^[1-5] +(error|warning) ')" "5" +check "suppressed findings stay out of table" "$(echo "$out" | grep -cE 'detected-jwt-token|detected-generic-secret')" "0" +check "legend omits suppressed-only CWEs" "$(echo "$out" | grep -cE 'CWE-321|CWE-798')" "0" +# cc-generic-catch fires 4x, so its multi-CWE cell appears on all 4 rows. +check "surfaces multi-CWE cells per row" "$(echo "$out" | grep -c 'CWE-396, CWE-636')" "4" +check "reports confidence" "$(echo "$out" | grep -cE 'cc-cljs-innerhtml.*HIGH')" "1" +check "legend expands a CWE id once" "$(echo "$out" | grep -c "CWE-636: Not Failing Securely")" "1" +check "legend expands an OWASP category" "$(echo "$out" | grep -c 'A10:2025 - Mishandling')" "1" +check "annotates the failure for GitHub" "$(echo "$out" | grep -c '::error::semgrep found 1')" "1" + +# --- warnings only: must NOT block ------------------------------------------- +jq '.runs |= map( + (.tool.driver.rules | map(select(.defaultConfiguration.level == "warning") | .id)) as $warn + | .results |= map(select(.ruleId as $i | $warn | index($i))))' \ + "${SAMPLE}" > "${WORK}/warn.sarif" +out="$(bash "${REPORT}" "${WORK}/warn.sarif" 2>&1)"; rc=$? +check "warnings alone do not block" "$rc" "0" +check "warning count still reported" "$(echo "$out" | grep -c '4 finding(s), 0 blocking')" "1" +check "no error annotation on warnings" "$(echo "$out" | grep -c '::error::')" "0" + +# --- suppressed-only: nothing actionable, must pass and say why --------------- +jq '.runs |= map(.results |= map(select(.suppressions != null)))' "${SAMPLE}" > "${WORK}/supp.sarif" +out="$(bash "${REPORT}" "${WORK}/supp.sarif" 2>&1)"; rc=$? +check "suppressed-only passes" "$rc" "0" +check "suppressed-only is not silent" "$(echo "$out" | grep -c '2 suppressed in source')" "1" + +# --- no findings -------------------------------------------------------------- +jq '.runs |= map(.results = [])' "${SAMPLE}" > "${WORK}/clean.sarif" +out="$(bash "${REPORT}" "${WORK}/clean.sarif" 2>&1)"; rc=$? +check "clean scan passes" "$rc" "0" +check "clean scan says so" "$(echo "$out" | grep -c 'no findings')" "1" + +# --- missing file: a scan that never ran must fail loudly -------------------- +out="$(bash "${REPORT}" "${WORK}/absent.sarif" 2>&1)"; rc=$? +check "missing SARIF fails" "$rc" "1" +check "missing SARIF explains why" "$(echo "$out" | grep -c 'did not complete')" "1" + +echo "report-sarif tests: ${pass} passed, ${fail} failed" +[ "${fail}" -eq 0 ] diff --git a/spec-fixtures/sarif/sample.sarif b/spec-fixtures/sarif/sample.sarif new file mode 100644 index 0000000..f769f44 --- /dev/null +++ b/spec-fixtures/sarif/sample.sarif @@ -0,0 +1,314 @@ +{ + "version": "2.1.0", + "runs": [ + { + "invocations": [ + { + "executionSuccessful": true, + "toolExecutionNotifications": [ + { + "descriptor": { + "id": "Syntax error" + }, + "level": "warning", + "message": { + "text": "Syntax error at line last-task-result.edn:290:\n `:` was unexpected" + } + }, + { + "descriptor": { + "id": "Syntax error" + }, + "level": "warning", + "message": { + "text": "Syntax error at line resources/email/order.html:29:\n `=\"padding-bottom: 9px\"` was unexpected" + } + } + ] + } + ], + "results": [ + { + "fingerprints": { + "matchBasedId/v1": "requires login" + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "spec/clj/cleancoders/jwplayer_spec.clj", + "uriBaseId": "%SRCROOT%" + }, + "region": { + "endColumn": 129, + "endLine": 37, + "startColumn": 17, + "startLine": 37 + } + } + } + ], + "message": { + "text": "JWT token detected" + }, + "properties": {}, + "ruleId": "generic.secrets.security.detected-jwt-token.detected-jwt-token", + "suppressions": [ + { + "kind": "inSource" + } + ] + }, + { + "fingerprints": { + "matchBasedId/v1": "requires login" + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/clj/cleancoders/config.clj", + "uriBaseId": "%SRCROOT%" + }, + "region": { + "endColumn": 63, + "endLine": 118, + "startColumn": 5, + "startLine": 118 + } + } + } + ], + "message": { + "text": "Generic Secret detected" + }, + "properties": {}, + "ruleId": "generic.secrets.security.detected-generic-secret.detected-generic-secret", + "suppressions": [ + { + "kind": "inSource" + } + ] + }, + { + "fingerprints": { + "matchBasedId/v1": "requires login" + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/clj/cleancoders/db_browser/engine.clj", + "uriBaseId": "%SRCROOT%" + }, + "region": { + "endColumn": 61, + "endLine": 58, + "startColumn": 38, + "startLine": 58 + } + } + } + ], + "message": { + "text": "A catch of Exception/Throwable that returns true or nil. When the guarded expression is a security decision this fails OPEN — the error path grants what the success path would have denied. Catch narrowly, log, and return the restrictive value. LOW PRECISION — many generic catches are legitimate; triage with /security-audit before acting." + }, + "properties": {}, + "ruleId": "Users.alex-root-roatch.current-projects.github-actions.security-rules.semgrep.cc-generic-catch" + }, + { + "fingerprints": { + "matchBasedId/v1": "requires login" + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/clj/cleancoders/db_browser/engine.clj", + "uriBaseId": "%SRCROOT%" + }, + "region": { + "endColumn": 66, + "endLine": 165, + "startColumn": 43, + "startLine": 165 + } + } + } + ], + "message": { + "text": "A catch of Exception/Throwable that returns true or nil. When the guarded expression is a security decision this fails OPEN — the error path grants what the success path would have denied. Catch narrowly, log, and return the restrictive value. LOW PRECISION — many generic catches are legitimate; triage with /security-audit before acting." + }, + "properties": {}, + "ruleId": "Users.alex-root-roatch.current-projects.github-actions.security-rules.semgrep.cc-generic-catch" + }, + { + "fingerprints": { + "matchBasedId/v1": "requires login" + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/clj/cleancoders/mfa.clj", + "uriBaseId": "%SRCROOT%" + }, + "region": { + "endColumn": 31, + "endLine": 218, + "startColumn": 8, + "startLine": 218 + } + } + } + ], + "message": { + "text": "A catch of Exception/Throwable that returns true or nil. When the guarded expression is a security decision this fails OPEN — the error path grants what the success path would have denied. Catch narrowly, log, and return the restrictive value. LOW PRECISION — many generic catches are legitimate; triage with /security-audit before acting." + }, + "properties": {}, + "ruleId": "Users.alex-root-roatch.current-projects.github-actions.security-rules.semgrep.cc-generic-catch" + }, + { + "fingerprints": { + "matchBasedId/v1": "requires login" + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/clj/cleancoders/web.clj", + "uriBaseId": "%SRCROOT%" + }, + "region": { + "endColumn": 31, + "endLine": 31, + "startColumn": 8, + "startLine": 31 + } + } + } + ], + "message": { + "text": "A catch of Exception/Throwable that returns true or nil. When the guarded expression is a security decision this fails OPEN — the error path grants what the success path would have denied. Catch narrowly, log, and return the restrictive value. LOW PRECISION — many generic catches are legitimate; triage with /security-audit before acting." + }, + "properties": {}, + "ruleId": "Users.alex-root-roatch.current-projects.github-actions.security-rules.semgrep.cc-generic-catch" + }, + { + "fingerprints": { + "matchBasedId/v1": "requires login" + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "src/cljs/cleancoders/contact.cljs", + "uriBaseId": "%SRCROOT%" + }, + "region": { + "endColumn": 38, + "endLine": 224, + "startColumn": 5, + "startLine": 224 + } + } + } + ], + "message": { + "text": "Assigning to .-innerHTML (or dommy/set-html!) parses the value as HTML. If it crosses a trust boundary this is DOM XSS. Use .-textContent, or sanitize with DOMPurify first." + }, + "properties": {}, + "ruleId": "Users.alex-root-roatch.current-projects.github-actions.security-rules.semgrep.cc-cljs-innerhtml" + } + ], + "tool": { + "driver": { + "name": "Semgrep OSS", + "rules": [ + { + "id": "Users.alex-root-roatch.current-projects.github-actions.security-rules.semgrep.cc-cljs-innerhtml", + "name": "Users.alex-root-roatch.current-projects.github-actions.security-rules.semgrep.cc-cljs-innerhtml", + "defaultConfiguration": { + "level": "error" + }, + "properties": { + "precision": "very-high", + "tags": [ + "CWE-79: Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting')", + "HIGH CONFIDENCE", + "OWASP-A05:2025 - Injection", + "security" + ] + }, + "shortDescription": { + "text": "Semgrep Finding: Users.alex-root-roatch.current-projects.github-actions.security-rules.semgrep.cc-cljs-innerhtml" + } + }, + { + "id": "Users.alex-root-roatch.current-projects.github-actions.security-rules.semgrep.cc-generic-catch", + "name": "Users.alex-root-roatch.current-projects.github-actions.security-rules.semgrep.cc-generic-catch", + "defaultConfiguration": { + "level": "warning" + }, + "properties": { + "precision": "very-high", + "tags": [ + "CWE-396: Declaration of Catch for Generic Exception", + "CWE-636: Not Failing Securely ('Failing Open')", + "LOW CONFIDENCE", + "OWASP-A10:2025 - Mishandling of Exceptional Conditions", + "security" + ] + }, + "shortDescription": { + "text": "Semgrep Finding: Users.alex-root-roatch.current-projects.github-actions.security-rules.semgrep.cc-generic-catch" + } + }, + { + "id": "generic.secrets.security.detected-generic-secret.detected-generic-secret", + "name": "generic.secrets.security.detected-generic-secret.detected-generic-secret", + "defaultConfiguration": { + "level": "error" + }, + "properties": { + "precision": "very-high", + "tags": [ + "CWE-798: Use of Hard-coded Credentials", + "LOW CONFIDENCE", + "OWASP-A07:2021 - Identification and Authentication Failures", + "OWASP-A07:2025 - Authentication Failures", + "security" + ] + }, + "shortDescription": { + "text": "Semgrep Finding: generic.secrets.security.detected-generic-secret.detected-generic-secret" + } + }, + { + "id": "generic.secrets.security.detected-jwt-token.detected-jwt-token", + "name": "generic.secrets.security.detected-jwt-token.detected-jwt-token", + "defaultConfiguration": { + "level": "error" + }, + "properties": { + "precision": "very-high", + "tags": [ + "CWE-321: Use of Hard-coded Cryptographic Key", + "LOW CONFIDENCE", + "OWASP-A02:2021 - Cryptographic Failures", + "OWASP-A04:2025 - Cryptographic Failures", + "security" + ] + }, + "shortDescription": { + "text": "Semgrep Finding: generic.secrets.security.detected-jwt-token.detected-jwt-token" + } + } + ], + "semanticVersion": "1.157.0" + } + } + } + ], + "$schema": "https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/schemas/sarif-schema-2.1.0.json" +}