From 109b114c71f78fd44084f33617759150dfaede75 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sun, 13 Sep 2026 13:01:22 +0900 Subject: [PATCH 1/2] fix(security-scan): use current OSV output flags and bind SARIF upload to the head checkout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #2132. The pinned ghcr.io/google/osv-scanner-action:v2.5.1 image warns on every run that `--output` is deprecated in favor of `--output-file` (scanner) and `--output-files` (reporter); a bare `--output-files=` defaults to the sarif format in v2.5.1 (cmd/osv-reporter/main.go: format := "sarif" unless a `format:` prefix is given), so the reporter output is byte-for-byte the same file. The exact base/head checkouts live in `source`, not the workspace root, so `upload-sarif` resolved commit identity against a non-repository and logged "does not appear to be a git repository" twice before falling back to the caller-supplied sha; `checkout_path` now names the real checkout. Contract: the new test pins the non-deprecated flags, rejects the deprecated ones, and derives the required `checkout_path` from the head checkout step's own `path:` (removing the binding makes it fail — verified RED before GREEN). Co-Authored-By: Claude Fable 5.1 --- .github/workflows/security-scan.yml | 14 ++++--- .../test_required_workflow_queue_contract.py | 37 +++++++++++++++++-- 2 files changed, 42 insertions(+), 9 deletions(-) diff --git a/.github/workflows/security-scan.yml b/.github/workflows/security-scan.yml index 500e22b4ab..e04d7bf8f3 100644 --- a/.github/workflows/security-scan.yml +++ b/.github/workflows/security-scan.yml @@ -164,7 +164,7 @@ jobs: with: scan-args: | --format=json - --output=old-results.json + --output-file=old-results.json --maven-registry=https://maven-central.storage-download.googleapis.com/maven2 --no-resolve --allow-no-lockfiles @@ -182,7 +182,7 @@ jobs: with: scan-args: | --format=json - --output=old-results.json + --output-file=old-results.json --no-resolve --allow-no-lockfiles -r @@ -215,7 +215,7 @@ jobs: with: scan-args: | --format=json - --output=new-results.json + --output-file=new-results.json --maven-registry=https://maven-central.storage-download.googleapis.com/maven2 --no-resolve --allow-no-lockfiles @@ -233,7 +233,7 @@ jobs: with: scan-args: | --format=json - --output=new-results.json + --output-file=new-results.json --no-resolve --allow-no-lockfiles -r @@ -286,7 +286,7 @@ jobs: uses: google/osv-scanner-action/osv-reporter-action@8e5cf47b818121e8b405931c82126c2630b0b20d # v2.3.8 with: scan-args: | - --output=results.sarif + --output-files=results.sarif --old=old-results.json --new=new-results.json --gh-annotations=true @@ -323,6 +323,10 @@ jobs: uses: github/codeql-action/upload-sarif@cdf488f595d80d6e07e03d4674febd5ab45fa938 # v4.37.9 with: sarif_file: results.sarif + # The exact head checkout lives in `source`, not the workspace root; + # without this binding upload-sarif logs "does not appear to be a git + # repository" twice and falls back to server-derived commit identity. + checkout_path: ${{ github.workspace }}/source # results.sarif is produced after checkout of the pull request head. # Uploading it against refs/pull/*/merge can race GitHub's synthetic # merge ref and fail with "commit_oid is not a merge commit". diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index 19fe6b0f7f..157556502a 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -1674,8 +1674,8 @@ def test_security_scan_preserves_base_output_across_cross_fork_checkout() -> Non assert workflow.count("--allow-no-lockfiles") == 4 assert workflow.count("path: source") == 2 - assert workflow.count("--output=old-results.json") == 2 - assert workflow.count("--output=new-results.json") == 2 + assert workflow.count("--output-file=old-results.json") == 2 + assert workflow.count("--output-file=new-results.json") == 2 assert workflow.count("source/") == 4 assert "clean: false" not in workflow assert "test -s old-results.json" in workflow @@ -1732,12 +1732,41 @@ def test_osv_scan_logs_and_retries_without_transitive_resolution_on_resolver_fai "Retry head OSV without transitive resolution\n if: steps.osv_head.outcome == 'failure'\n continue-on-error: true" in workflow ) - assert "--output=old-results.json" in workflow - assert "--output=new-results.json" in workflow + assert "--output-file=old-results.json" in workflow + assert "--output-file=new-results.json" in workflow assert "Print OSV findings being compared" in workflow assert "OSV {label} scan produced {len(findings)} finding(s)" in workflow +def test_osv_scan_uses_current_output_flags_and_binds_sarif_checkout_path() -> None: + """Drop deprecated OSV output flags and bind upload-sarif to the real checkout. + + Live evidence (ContextualWisdomLab/.github#2132): the pinned + `ghcr.io/google/osv-scanner-action:v2.5.1` image warns + `--output has been deprecated in favor of --output-file` (scanner) and + `... in favor of --output-files` (reporter), and `upload-sarif` logged + twice that the workspace root "does not appear to be a git repository" + because the exact head is checked out into `source`. A bare + `--output-files=` defaults to the sarif format in v2.5.1, so the + reporter's output is unchanged. The checkout-path assertion is the + negative fixture: an absent or wrong `checkout_path` fails here instead + of silently relying on server-derived commit identity. + """ + workflow = workflow_text("security-scan.yml") + + assert workflow.count("--output-file=old-results.json") == 2 + assert workflow.count("--output-file=new-results.json") == 2 + assert "--output-files=results.sarif" in workflow + assert "--output=old-results.json" not in workflow + assert "--output=new-results.json" not in workflow + assert "--output=results.sarif" not in workflow + + head_checkout = workflow_step(workflow, "Checkout head") + checkout_dir = re.search(r"(?m)^\s+path: (\S+)$", head_checkout).group(1) + upload_step = workflow_step(workflow, "Upload OSV SARIF to code scanning") + assert f"checkout_path: ${{{{ github.workspace }}}}/{checkout_dir}" in upload_step + + def test_osv_sarif_upload_is_marked_comprehensive_after_clean_comparison( tmp_path: Path, ) -> None: From 7e5b971a48dfdbd27d98ca8c39662c1d948935b4 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sun, 13 Sep 2026 17:16:09 +0900 Subject: [PATCH 2/2] test(security-scan): assert each OSV step's output flag individually CodeRabbit on #2143: whole-workflow counts could pass if the same string appeared in another step or log line while a scanner or reporter step lost its flag. Check the four scan/retry steps and the reporter step by name. Co-Authored-By: Claude Fable 5.1 --- tests/test_required_workflow_queue_contract.py | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index 157556502a..87277d45f5 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -1754,12 +1754,18 @@ def test_osv_scan_uses_current_output_flags_and_binds_sarif_checkout_path() -> N """ workflow = workflow_text("security-scan.yml") - assert workflow.count("--output-file=old-results.json") == 2 - assert workflow.count("--output-file=new-results.json") == 2 - assert "--output-files=results.sarif" in workflow - assert "--output=old-results.json" not in workflow - assert "--output=new-results.json" not in workflow - assert "--output=results.sarif" not in workflow + # Check each named scanner/reporter step on its own, so a flag removed from + # one step cannot hide behind the same string appearing elsewhere. + for step_name, output_flag in ( + ("Scan base with OSV", "--output-file=old-results.json"), + ("Retry base OSV without transitive resolution", "--output-file=old-results.json"), + ("Scan head with OSV", "--output-file=new-results.json"), + ("Retry head OSV without transitive resolution", "--output-file=new-results.json"), + ("Report PR-introduced OSV findings", "--output-files=results.sarif"), + ): + step = workflow_step(workflow, step_name) + assert output_flag in step, step_name + assert "\n --output=" not in step, step_name head_checkout = workflow_step(workflow, "Checkout head") checkout_dir = re.search(r"(?m)^\s+path: (\S+)$", head_checkout).group(1)