From e19b8023d4dc792b794f554c73e1892eca1aa865 Mon Sep 17 00:00:00 2001 From: "Jonathan D.A. Jewell" <6759885+hyperpolymath@users.noreply.github.com> Date: Tue, 6 Oct 2026 15:34:43 +0100 Subject: [PATCH] fix(scorecard): reconcile action-pin findings per workflow file reconcile-scorecard-actions-lock.rb raised on the first workflow that failed native action-lock verification. The CLI then exited 2 without writing results.reconciled.sarif, so scorecard-reusable.yml uploaded the RAW SARIF and every lock-pinned action in every workflow reappeared as a code-scanning alert. Measured on metadatastician/ZenodoDeposits.jl#15 (run 37472579223): 51 PinnedDependencies alerts, 48 of them in workflows the PR never touched, because quality.yml (deliberately desynced as a positive control) failed verification. Verification is now confined to the workflow that fails: - verify_workflow() wraps the job-level-ref lock check, the gh call and JSON parsing; any failure there (or unparseable workflow YAML) is recorded for that workflow only, and its findings are kept. - reconcile() returns [document, audit, failures]; existing two-value destructuring callers are unaffected. - CLI: on a partial failure it writes OUTPUT and AUDIT_JSON, logs each failed workflow and exits 3, so the workflow uploads the reconciled SARIF (only the failing workflow's alerts) and the run still fails by design. Usage/SARIF-structure errors still exit 2 with no OUTPUT. Nothing is weakened: a workflow that fails verification still surfaces all of its alerts and still fails the run. Two tests that asserted a raise now assert the stronger per-file contract (finding kept, no audit record, failure reported). New tests cover a failing workflow ahead of a verified one, an unparseable workflow, and the CLI exit/file contract. Mutants (re-raise; stop filtering after first failure; the pre-fix origin/main script) are each killed. Refs #1036 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01X3hgXxWm6umMgZkjYyHnnm --- scripts/reconcile-scorecard-actions-lock.rb | 91 ++++++++++--- .../reconcile-scorecard-actions-lock-test.rb | 126 ++++++++++++++++-- 2 files changed, 187 insertions(+), 30 deletions(-) diff --git a/scripts/reconcile-scorecard-actions-lock.rb b/scripts/reconcile-scorecard-actions-lock.rb index 8cc4dedf2..30f196ce3 100644 --- a/scripts/reconcile-scorecard-actions-lock.rb +++ b/scripts/reconcile-scorecard-actions-lock.rb @@ -3,6 +3,12 @@ # Reconcile only Scorecard's inline action-pin findings with native lock coverage. # No lock semantics are reimplemented: gh actions-lock verifies each workflow. # Usage: ruby reconcile-scorecard-actions-lock.rb INPUT OUTPUT AUDIT_JSON [ROOT] +# Exit codes: +# 0 every eligible workflow verified; OUTPUT and AUDIT_JSON written. +# 3 OUTPUT and AUDIT_JSON written, but at least one workflow failed native +# verification. Only that workflow's findings are retained; findings in +# verified workflows are still removed. Callers must treat this as failure. +# 2 usage or SARIF structure error; OUTPUT is NOT written. require 'json' require 'open3' require 'yaml' @@ -80,6 +86,11 @@ def self.lock_covers_reusable_deps?(lock_path, relative, reusable_deps) end end + # Decide whether one workflow's `gh actions-lock --verify` result is clean + # enough to suppress its Scorecard pin findings. Accepts a valid result whose + # findings are empty (with a zero exit) or only `sha-as-ref`, and an invalid + # result whose only non-`sha-as-ref` findings are `stale` entries for + # job-level reusable refs the verifier cannot see (standards#1036). def self.verification_accepted?(verification, status, reusable_deps) return false unless verification.is_a?(Hash) && verification['findings'].is_a?(Array) findings = verification['findings'] @@ -103,22 +114,52 @@ def self.verification_accepted?(verification, status, reusable_deps) accepted_stale.positive? end + # Verify one workflow (+relative+ to +root+) against `actions.lock`: job-level + # reusable refs must be recorded in the lock, and `gh actions-lock --verify` + # must report an accepted result. Returns the verifier's JSON on success. + # Raises with the reason on any failure, including unparseable workflow YAML, + # a missing `gh`, or non-JSON verifier output, so the caller can confine the + # failure to this one file. + def self.verify_workflow(root, relative) + path = File.join(root, relative) + lock_path = File.join(root, '.github/workflows/actions.lock') + reusable_deps = reusable_workflow_deps(path) + unless lock_covers_reusable_deps?(lock_path, relative, reusable_deps) + raise "Native action-lock verification failed for #{relative}: job-level reusable ref absent from actions.lock" + end + + stdout, stderr, status = Open3.capture3('gh', 'actions-lock', relative, + '--verify', '--no-interactive', '--json=valid,findings', chdir: root) + warn stderr unless stderr.empty? + verification = JSON.parse(stdout) + unless verification_accepted?(verification, status, reusable_deps) + raise "Native action-lock verification failed for #{relative}" + end + verification + end + # Remove Scorecard action-pin findings only when they identify a remote action # entry in a regular workflow below +root+ and native action-lock verification - # succeeds. The supplied SARIF document is updated in place and returned with - # audit records for removed findings. + # of THAT workflow succeeds. Verification is per file: a workflow that fails + # keeps all of its findings and is recorded in the returned failures map + # (relative path => reason), while findings in other, verified workflows are + # still removed. The supplied SARIF document is updated in place and returned + # as [document, audit, failures], where audit lists the removed findings. # - # Invokes +gh actions-lock+ once per eligible workflow. Raises when the SARIF - # structure or native verification result is invalid, or verification fails. + # Invokes +gh actions-lock+ at most once per eligible workflow. Raises only + # when the SARIF structure itself is invalid. def self.reconcile(document, root) raise 'Expected a SARIF 2.1.0 document with runs' unless document.is_a?(Hash) && document['version'] == '2.1.0' && document['runs'].is_a?(Array) && !document['runs'].empty? + document['runs'].each do |run| + raise 'Expected a results array' unless run.is_a?(Hash) && run['results'].is_a?(Array) + end root = File.realpath(root) verified = {} + failures = {} audit = [] document['runs'].each do |run| - raise 'Expected a results array' unless run['results'].is_a?(Array) next unless run.dig('tool', 'driver', 'name') == 'Scorecard' run['results'] = run['results'].reject do |result| @@ -135,23 +176,25 @@ def self.reconcile(document, root) path = File.join(root, relative) next false unless File.file?(path) && !File.symlink?(path) && File.realpath(path).start_with?(root + '/') && File.file?(File.join(root, '.github/workflows/actions.lock')) - next false unless action_lines(path).include?(line) + # A failure in one workflow is confined to that workflow: its findings + # are kept and every other workflow is still reconciled on its own. + next false if failures.key?(relative) + + begin + lines = action_lines(path) + rescue StandardError => error + failures[relative] = "Unparseable workflow #{relative}: #{error.message}" + next false + end + next false unless lines.include?(line) unless verified.key?(relative) - lock_path = File.join(root, '.github/workflows/actions.lock') - reusable_deps = reusable_workflow_deps(path) - unless lock_covers_reusable_deps?(lock_path, relative, reusable_deps) - raise "Native action-lock verification failed for #{relative}: job-level reusable ref absent from actions.lock" + begin + verified[relative] = verify_workflow(root, relative) + rescue StandardError => error + failures[relative] = error.message + next false end - - stdout, stderr, status = Open3.capture3('gh', 'actions-lock', relative, - '--verify', '--no-interactive', '--json=valid,findings', chdir: root) - warn stderr unless stderr.empty? - verification = JSON.parse(stdout) - unless verification_accepted?(verification, status, reusable_deps) - raise "Native action-lock verification failed for #{relative}" - end - verified[relative] = verification end audit << { 'file' => relative, 'line' => line, 'rule' => result['ruleId'], 'reason' => 'False positive: native direct and transitive pins verified by gh actions-lock --verify', @@ -159,7 +202,7 @@ def self.reconcile(document, root) true end end - [document, audit] + [document, audit, failures] end end @@ -168,10 +211,16 @@ def self.reconcile(document, root) input, output, audit_path, root = ARGV raise 'Usage: INPUT OUTPUT AUDIT_JSON [ROOT]' unless input && output && audit_path && ARGV.length <= 4 raise 'Keep the original SARIF as a separate artifact' if File.expand_path(input) == File.expand_path(output) - document, audit = ScorecardActionsLock.reconcile(JSON.parse(File.read(input)), root || Dir.pwd) + document, audit, failures = ScorecardActionsLock.reconcile(JSON.parse(File.read(input)), root || Dir.pwd) File.write(output, JSON.pretty_generate(document) + "\n") File.write(audit_path, JSON.pretty_generate(audit) + "\n") puts "Reconciled #{audit.length} verified native action-pin false positives; all other findings retained." + unless failures.empty? + failures.each { |relative, reason| warn "Scorecard reconciliation kept findings for #{relative}: #{reason}" } + warn "Scorecard reconciliation incomplete: #{failures.length} workflow(s) failed native action-lock " \ + 'verification; their findings are retained in the reconciled SARIF.' + exit 3 + end rescue StandardError => error warn "Scorecard reconciliation failed: #{error.message}" exit 2 diff --git a/scripts/tests/reconcile-scorecard-actions-lock-test.rb b/scripts/tests/reconcile-scorecard-actions-lock-test.rb index 152fac3d6..43f614618 100644 --- a/scripts/tests/reconcile-scorecard-actions-lock-test.rb +++ b/scripts/tests/reconcile-scorecard-actions-lock-test.rb @@ -4,9 +4,14 @@ require 'tmpdir' require 'fileutils' require 'minitest/autorun' +require 'open3' +require 'rbconfig' require_relative '../reconcile-scorecard-actions-lock' class ScorecardActionsLockTest < Minitest::Test + # Build a temporary repository root with one workflow, a placeholder lock and + # a stub `gh` on PATH that records invocations and replays canned verifier + # output (shared, or per workflow from responses/). def setup @root = Dir.mktmpdir('scorecard-lock-') @old_path = ENV.fetch('PATH') @@ -20,6 +25,14 @@ def setup actions-lock\\ .github/workflows/*.yml\\ --verify\\ --no-interactive\\ --json=valid,findings) ;; *) exit 97 ;; esac + # Per-workflow responses: responses/.json (+ .exit) override + # the shared TEST_LOCK_JSON / TEST_LOCK_EXIT, so one run can mix a + # verifying workflow with a failing one. + name=$(basename "$2") + if [ -f "responses/$name.json" ]; then + cat "responses/$name.json" + exit "$(cat "responses/$name.exit" 2>/dev/null || echo 0)" + fi printf '%s' "$TEST_LOCK_JSON" exit "${TEST_LOCK_EXIT:-0}" SH @@ -65,14 +78,27 @@ def test_missing_lock_and_foreign_tools_are_not_filtered refute File.exist?(File.join(@root, 'invocation')) end + # A verifier result that is malformed, invalid or paired with a non-zero exit + # never removes a finding: the finding is kept, no audit record is written, + # and the workflow is reported as a failure. def test_invalid_verification_never_becomes_a_clean_result ['{}', '{"valid":false,"findings":[]}', '{"valid":true}', '[]', 'not JSON'].each do |invalid| ENV['TEST_LOCK_JSON'] = invalid - assert_raises(StandardError) { ScorecardActionsLock.reconcile(document(finding), @root) } + assert_verification_failed(document(finding), '.github/workflows/ci.yml') end ENV['TEST_LOCK_JSON'] = '{"valid":true,"findings":[]}' ENV['TEST_LOCK_EXIT'] = '1' - assert_raises(StandardError) { ScorecardActionsLock.reconcile(document(finding), @root) } + assert_verification_failed(document(finding), '.github/workflows/ci.yml') + end + + # Assert that reconciling +doc+ removes nothing, records no audit entry and + # reports +relative+ as a failed workflow. + def assert_verification_failed(doc, relative) + original = JSON.parse(JSON.generate(doc)) + result, audit, failures = ScorecardActionsLock.reconcile(doc, @root) + assert_equal original, result + assert_empty audit + assert_equal [relative], failures.keys end def test_workflow_symlinks_are_not_filtered @@ -132,7 +158,10 @@ def test_mixed_workflow_with_job_level_reusable_ref_is_not_rejected_as_stale assert_equal 1, audit.length end - def test_arm_d_job_level_reusable_ref_absent_from_lock_raises + # Arm D of standards#1036: a job-level reusable ref absent from actions.lock + # is a verification failure even though gh actions-lock passes it vacuously; + # the finding is kept and the workflow reported, never silently dropped. + def test_arm_d_job_level_reusable_ref_absent_from_lock_is_kept_and_reported missing_sha = 'deadbeefdeadbeefdeadbeefdeadbeefdeadbeef' File.write(File.join(@root, '.github/workflows/scorecard.yml'), <<~YAML) name: Scorecard @@ -150,11 +179,90 @@ def test_arm_d_job_level_reusable_ref_absent_from_lock_raises ENV['TEST_LOCK_JSON'] = '{"valid":true,"findings":[]}' ENV['TEST_LOCK_EXIT'] = '0' - assert_raises(StandardError) do - ScorecardActionsLock.reconcile( - document(finding(path: '.github/workflows/scorecard.yml', line: 5)), - @root - ) - end + assert_verification_failed(document(finding(path: '.github/workflows/scorecard.yml', line: 5)), + '.github/workflows/scorecard.yml') + end + # Write a second workflow and a per-file verifier response that FAILS for it + # (a real lock desync, as on metadatastician/ZenodoDeposits.jl quality.yml). + def add_failing_workflow(name = 'quality.yml') + File.write(File.join(@root, ".github/workflows/#{name}"), + "jobs:\n lint:\n steps:\n - uses: actions/checkout@v7.0.0\n") + FileUtils.mkdir_p(File.join(@root, 'responses')) + File.write(File.join(@root, "responses/#{name}.json"), JSON.generate( + 'valid' => false, + 'findings' => [{ 'category' => 'ref-changed', 'severity' => 'error', + 'dependency' => 'actions/checkout@v7.0.0' }] + )) + File.write(File.join(@root, "responses/#{name}.exit"), '1') + ".github/workflows/#{name}" + end + + # The regression for the whole-document abort: one workflow failing native + # verification must not stop reconciliation of any other workflow. The + # failing file comes FIRST so an abort (or a "stop filtering after the first + # failure" variant) leaves the verified file's findings in place. + def test_one_failing_workflow_does_not_unreconcile_the_others + bad = add_failing_workflow + bad_findings = [finding(path: bad, line: 4), finding(path: bad, line: 4)] + good_findings = [finding, finding] + result, audit, failures = ScorecardActionsLock.reconcile(document(*bad_findings, *good_findings), @root) + + assert_equal bad_findings, result['runs'][0]['results'], 'failing workflow keeps every finding; verified one loses its' + assert_equal ['.github/workflows/ci.yml'] * 2, audit.map { |a| a['file'] } + assert_equal [bad], failures.keys + assert_match(/Native action-lock verification failed for #{Regexp.escape(bad)}/, failures[bad]) + invocations = File.readlines(File.join(@root, 'invocation')) + assert_equal 2, invocations.length, 'gh runs once per workflow, failures included' + end + + # A workflow whose YAML cannot be parsed is a failure of that workflow only. + def test_unparseable_workflow_is_confined_to_itself + File.write(File.join(@root, '.github/workflows/broken.yml'), "jobs: [unclosed\n") + broken = finding(path: '.github/workflows/broken.yml', line: 1) + result, audit, failures = ScorecardActionsLock.reconcile(document(broken, finding), @root) + assert_equal [broken], result['runs'][0]['results'] + assert_equal 1, audit.length + assert_equal ['.github/workflows/broken.yml'], failures.keys + end + + # Run the script as scorecard-reusable.yml does and return [status, stderr]. + def run_cli(input_doc, out, audit) + input = File.join(@root, 'results.sarif') + File.write(input, input_doc.is_a?(String) ? input_doc : JSON.generate(input_doc)) + script = File.expand_path('../reconcile-scorecard-actions-lock.rb', __dir__) + _, stderr, status = Open3.capture3(RbConfig.ruby, script, input, out, audit, @root) + [status, stderr] + end + + # Workflow contract: on a partial failure the reconciled SARIF IS written (so + # the select step uploads it, not the raw SARIF) and the exit is non-zero (so + # the run still fails by design). + def test_cli_partial_failure_writes_output_and_exits_nonzero + bad = add_failing_workflow + out = File.join(@root, 'out.sarif') + audit = File.join(@root, 'audit.json') + status, stderr = run_cli(document(finding(path: bad, line: 4), finding), out, audit) + assert_equal 3, status.exitstatus + assert File.size?(out), 'reconciled SARIF must be written on a partial failure' + kept = JSON.parse(File.read(out))['runs'][0]['results'] + assert_equal [bad], kept.map { |r| r.dig('locations', 0, 'physicalLocation', 'artifactLocation', 'uri') } + assert_equal 1, JSON.parse(File.read(audit)).length + assert_match(/kept findings for #{Regexp.escape(bad)}/, stderr) + end + + # Workflow contract: success exits 0; a structural error exits 2 and writes + # no output, so the workflow falls back to the raw SARIF and fails. + def test_cli_success_and_structural_error_exit_codes + out = File.join(@root, 'out.sarif') + audit = File.join(@root, 'audit.json') + status, = run_cli(document(finding), out, audit) + assert_equal 0, status.exitstatus + assert_empty JSON.parse(File.read(out))['runs'][0]['results'] + + File.unlink(out) + status, stderr = run_cli('{"version":"2.0.0","runs":[]}', out, audit) + assert_equal 2, status.exitstatus + refute File.exist?(out), 'no reconciled SARIF on a structural error' + assert_match(/Scorecard reconciliation failed/, stderr) end end