From 109b114c71f78fd44084f33617759150dfaede75 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sun, 13 Sep 2026 13:01:22 +0900 Subject: [PATCH 1/5] 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 8db9a1db797c51be7feb576513bc80c9df5540d1 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sun, 13 Sep 2026 13:04:42 +0900 Subject: [PATCH 2/5] fix(sast-semgrep): fold the changed-scope gate into its single consumer job One consumer, one runner: the standalone `changed-scope` job cost a second runner allocation per PR org-wide purely to compute two booleans for the `semgrep` job. The classifier now runs as a step inside `semgrep` (after harden-runner), the expensive steps gate on `steps.scope.outputs.code`, and the enforce step carries the same guard so a step-skipped scan's empty `rc` cannot fail a doc-only PR. The job keeps `if: github.event.action != 'closed'` with no needs-output term, so a doc-only run still concludes `success` (required-workflow-path-filter-boundary.md). strix.yml is left alone (hot-file collision zone). Measured in #1904; contract tests updated. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/sast-semgrep.yml | 50 ++++++++----------- docs/product-technical-gap-baseline.md | 21 +++++++- tests/test_docs_only_pr_runner_admission.py | 50 ++++++++++++++++--- ...required_security_runner_image_contract.py | 8 +-- 4 files changed, 89 insertions(+), 40 deletions(-) diff --git a/.github/workflows/sast-semgrep.yml b/.github/workflows/sast-semgrep.yml index 12b7013da3..283eac0097 100644 --- a/.github/workflows/sast-semgrep.yml +++ b/.github/workflows/sast-semgrep.yml @@ -38,8 +38,8 @@ permissions: contents: read jobs: - changed-scope: - name: Detect changed scope + semgrep: + name: Semgrep (multi-language SAST) # The org ruleset IGNORES every `on:` filter (paths, branches, types) when it # runs this workflow in another repository, and a trigger-level skip would # leave `.github`'s classic required contexts Pending forever. Both @@ -47,16 +47,26 @@ jobs: # here and consumed through `needs`. See # docs/doctoring/required-workflow-path-filter-boundary.md. # Fails OPEN: an unreadable, empty, or truncated file list scans everything. + # The gate lives inside this job as a step-level guard (one runner, not two). if: github.event.action != 'closed' runs-on: ubuntu-24.04 - timeout-minutes: 5 permissions: contents: read pull-requests: read - outputs: - code: ${{ steps.scope.outputs.code }} - deps: ${{ steps.scope.outputs.deps }} + security-events: write + actions: read + env: + # Deterministic, no telemetry: registry rules are fetched but no scan data + # is sent back. + SEMGREP_SEND_METRICS: "off" + # Semgrep OSS 1.169.0. Keep the immutable manifest reference in one + # place so hosted scans and local reproduction cannot drift. + SEMGREP_IMAGE: "semgrep/semgrep@sha256:2b33f46ba66cf8cc2ad59ccfa7d22951fd00c632c38f1339e84ec8e6e641a942" steps: + - name: Harden the runner (Audit all outbound calls) + uses: step-security/harden-runner@b09bb98e06d4d774595224525879c09bc6e98c40 # v2.20.1 + with: + egress-policy: audit - name: Classify changed paths id: scope env: @@ -99,35 +109,15 @@ jobs: echo "code=${code}" >> "$GITHUB_OUTPUT" echo "deps=${deps}" >> "$GITHUB_OUTPUT" echo "changed-scope code=${code} deps=${deps}" - - semgrep: - name: Semgrep (multi-language SAST) - needs: changed-scope - if: github.event.action != 'closed' && needs.changed-scope.outputs.code == 'true' - runs-on: ubuntu-24.04 - permissions: - contents: read - security-events: write - actions: read - env: - # Deterministic, no telemetry: registry rules are fetched but no scan data - # is sent back. - SEMGREP_SEND_METRICS: "off" - # Semgrep OSS 1.169.0. Keep the immutable manifest reference in one - # place so hosted scans and local reproduction cannot drift. - SEMGREP_IMAGE: "semgrep/semgrep@sha256:2b33f46ba66cf8cc2ad59ccfa7d22951fd00c632c38f1339e84ec8e6e641a942" - steps: - - name: Harden the runner (Audit all outbound calls) - uses: step-security/harden-runner@b09bb98e06d4d774595224525879c09bc6e98c40 # v2.20.1 - with: - egress-policy: audit - name: Checkout exact submitted revision + if: steps.scope.outputs.code == 'true' uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: repository: ${{ github.event.pull_request.head.repo.full_name || github.repository }} ref: ${{ github.event.pull_request.head.sha || github.sha }} persist-credentials: false - name: Verify exact submitted revision + if: steps.scope.outputs.code == 'true' env: EXPECTED_CHECKOUT_REPOSITORY: ${{ github.event.pull_request.head.repo.full_name || github.repository }} EXPECTED_CHECKOUT_SHA: ${{ github.event.pull_request.head.sha || github.sha }} @@ -140,6 +130,7 @@ jobs: fi echo "SAST_CHECKOUT scanner=semgrep repository=${EXPECTED_CHECKOUT_REPOSITORY} expected_sha=${EXPECTED_CHECKOUT_SHA} actual_sha=${actual_sha}" - name: Verify pinned Semgrep manifest + if: steps.scope.outputs.code == 'true' run: | set -euo pipefail if [[ "${SEMGREP_IMAGE}" =~ ^semgrep/semgrep@sha256:[0-9a-f]{64}$ ]]; then @@ -151,6 +142,7 @@ jobs: fi - name: Run Semgrep (SARIF) id: semgrep + if: steps.scope.outputs.code == 'true' run: | set +e echo "Using ${SEMGREP_IMAGE}" @@ -219,7 +211,7 @@ jobs: echo "SEMGREP_ENGINE_FAILURE rc=${SEMGREP_RC:-missing}: Semgrep failed without a WARNING/ERROR SARIF result; inspect the scan command output above." fi - name: Enforce Semgrep gate (fail on Medium+ findings) - if: always() && (steps.semgrep_sarif.outputs.finding_count != '0' || steps.semgrep.outputs.rc != '0') + if: always() && steps.scope.outputs.code == 'true' && (steps.semgrep_sarif.outputs.finding_count != '0' || steps.semgrep.outputs.rc != '0') env: SEMGREP_RC: ${{ steps.semgrep.outputs.rc }} SEMGREP_FINDING_COUNT: ${{ steps.semgrep_sarif.outputs.finding_count }} diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 1cc9e20313..966ac7ef25 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -3237,8 +3237,8 @@ intended contract before rewriting the assertion — left for a dedicated follow ## Items 15/16/17 measurement: `Detect changed scope` gate jobs — 2 of 3 are pure runner overhead — 2026-09-05 -**Status:** Measured, not yet fixed. Recorded so the fix is grounded in real numbers rather than the intuition -this measurement partly refuted. +**Status:** Measured 2026-09-05; `sast-semgrep.yml` fixed 2026-09-13 (below); `strix.yml` deferred. Recorded so +the fix is grounded in real numbers rather than the intuition this measurement partly refuted. **Why measured.** Items 15/16/17 ask to remove needlessly-triggered workflows, consolidate workflow files ("bootup에도 시간이 듦"), and cut redundant steps; the standing complaint is the org's 60-concurrent-job @@ -3353,3 +3353,20 @@ queries the check-runs API at its own time, order-independently. The implementin their change was safe because they had scoped it narrowly, not because they had checked for the name collision — which is the more useful lesson: **a job name is unique only within one workflow file, and the same name in another file can carry the opposite safety property.** + +**Fixed for `sast-semgrep.yml`, 2026-09-13.** The standalone `changed-scope` job is gone; its +"Classify changed paths" step now runs inside the single consumer `semgrep` (after `harden-runner`, +which must audit the classifier's own `gh api` egress) and the four expensive steps plus the final +"Enforce Semgrep gate" step carry `steps.scope.outputs.code == 'true'`. The job keeps +`if: github.event.action != 'closed'` with no `needs.` term, so a doc-only PR's run still executes one +job that concludes `success` -- the load-bearing property from +[`required-workflow-path-filter-boundary.md`](doctoring/required-workflow-path-filter-boundary.md) is +preserved, and neither `Detect changed scope` nor `Semgrep (multi-language SAST)` is among `.github`'s +classic required contexts, so nothing goes Pending there. One trap the first draft would have shipped: +the enforce step's `always() && (... || steps.semgrep.outputs.rc != '0')` evaluates `rc` as the empty +string when `Run Semgrep` is step-skipped, which is `!= '0'` and would have failed every doc-only PR; +the guard on that step is what makes the fold safe. Net: one runner allocation per PR for this +workflow instead of two, org-wide. `strix.yml` (the other single-consumer gate) is deliberately left +alone -- it is a documented multi-PR hot-file collision zone. Contract: +`tests/test_docs_only_pr_runner_admission.py::test_sast_semgrep_folds_the_gate_into_its_single_consumer_at_step_level`, +`tests/test_required_security_runner_image_contract.py`. diff --git a/tests/test_docs_only_pr_runner_admission.py b/tests/test_docs_only_pr_runner_admission.py index 49631d2a19..674b984b63 100644 --- a/tests/test_docs_only_pr_runner_admission.py +++ b/tests/test_docs_only_pr_runner_admission.py @@ -31,9 +31,11 @@ WORKFLOWS_DIR = REPO_ROOT / ".github/workflows" # The required workflows that keep the canonical `changed-scope` gate job. +# `sast-semgrep.yml` has only one consumer job, so it folds the classifier +# into that job as a step-level guard instead of a standalone job -- see +# GATED_JOBS below. GATE_WORKFLOWS = ( "security-scan.yml", - "sast-semgrep.yml", "strix.yml", ) @@ -52,7 +54,6 @@ # output, keyed by workflow filename. GATED_JOBS = { "security-scan.yml": ("osv-scan", "dependency-review", "trivy-fs", "scorecard"), - "sast-semgrep.yml": ("semgrep",), "strix.yml": ("strix",), } @@ -86,7 +87,7 @@ def _on_block(workflow: str) -> str: def test_gate_job_is_byte_identical_across_the_five_workflows_apart_from_if(): - """The `changed-scope` block must not drift between its five copies.""" + """The `changed-scope` block must not drift between every gate copy.""" normalized_blocks = set() for filename in GATE_WORKFLOWS: workflow = _read(filename) @@ -110,7 +111,7 @@ def test_gate_job_and_codeql_scope_step_share_one_doc_pattern_line(): `COPYING.txt`/`NOTICE`/`NOTICE.txt` names. """ doc_pattern_lines = set() - for filename in (*GATE_WORKFLOWS, "codeql-pr.yml"): + for filename in (*GATE_WORKFLOWS, "sast-semgrep.yml", "codeql-pr.yml"): workflow = _read(filename) matches = [ line for line in workflow.splitlines() if "*.md|*.markdown" in line @@ -208,8 +209,8 @@ def test_codeql_pr_gates_analyze_head_at_step_level_not_job_level(): def test_each_gate_workflow_keeps_an_always_admitted_job(): """A fully-skipped run must conclude `success`, never `skipped`. - Every one of the five workflows needs at least one job with no `needs:` - and no needs-output-dependent `if:` -- the `changed-scope` job itself + Every gate workflow needs at least one job with no `needs:` and no + needs-output-dependent `if:` -- the `changed-scope` job itself qualifies -- so a doc-only PR's run still has a job that runs and succeeds instead of every job skipping and the run itself reporting `skipped` (an undocumented conclusion for a required check). @@ -220,3 +221,40 @@ def test_each_gate_workflow_keeps_an_always_admitted_job(): job_if = re.search(r"(?m)^ if: (.*)$", block) assert job_if is not None, filename assert "needs." not in job_if.group(1), filename + + +def test_sast_semgrep_folds_the_gate_into_its_single_consumer_at_step_level(): + """`sast-semgrep.yml` has one consumer, so the gate is a step, not a job. + + A standalone `changed-scope` job cost a second runner allocation per PR + purely to compute two booleans for one downstream job (measured in + docs/product-technical-gap-baseline.md, "Items 15/16/17 measurement"). + Folding it into `semgrep` keeps the load-bearing property -- the job + still runs and concludes `success` on a doc-only PR -- while the + expensive steps gate on the classifier step's output. The final gate + step must also carry that guard: a step-skipped `Run Semgrep` leaves + `steps.semgrep.outputs.rc` empty, which is `!= '0'`. + """ + workflow = _read("sast-semgrep.yml") + # The classifier's own log lines keep saying "changed-scope" (byte-for-byte + # verbatim across every copy, see test_gate_job_and_codeql_scope_step_share_ + # one_doc_pattern_line); what must be gone is the standalone JOB. + assert "changed-scope:" not in workflow + assert "needs: changed-scope" not in workflow + assert "needs.changed-scope" not in workflow + assert workflow.count("runs-on: ubuntu-24.04") == 1 + + semgrep = _top_level_job_block(workflow, "semgrep") + assert not re.search(r"(?m)^ needs:", semgrep) + job_if = re.search(r"(?m)^ if: (.*)$", semgrep) + assert job_if is not None + assert job_if.group(1) == "github.event.action != 'closed'" + assert "pull-requests: read" in semgrep + assert "id: scope" in semgrep + assert semgrep.count("steps.scope.outputs.code == 'true'") == 5 + assert ( + "if: always() && steps.scope.outputs.code == 'true' && " + "(steps.semgrep_sarif.outputs.finding_count != '0' || steps.semgrep.outputs.rc != '0')" + ) in semgrep + # Harden-runner audits egress and must precede the classifier's gh api call. + assert semgrep.index("Harden the runner") < semgrep.index("Classify changed paths") diff --git a/tests/test_required_security_runner_image_contract.py b/tests/test_required_security_runner_image_contract.py index 2b48f66251..d20c0c3a98 100644 --- a/tests/test_required_security_runner_image_contract.py +++ b/tests/test_required_security_runner_image_contract.py @@ -28,13 +28,15 @@ def test_sast_semgrep_uses_explicit_supported_image(self) -> None: `#1656` removed the sibling `cancel-closed-pr-runs` no-op job (it only duplicated PR-stable workflow concurrency), leaving one runner - job in this workflow instead of two. It is 2, not 1, again after the + job in this workflow instead of two. It was 2, not 1, again after the `changed-scope` gate job was added to skip doc-only PR scope (org - ruleset 18156473 ignores trigger-level path filters). + ruleset 18156473 ignores trigger-level path filters). The count + returned to 1 when that `changed-scope` job was folded into the + `semgrep` job as a step-level guard (one consumer, one runner). """ workflow = SAST_SEMGREP.read_text(encoding="utf-8") self.assertNotIn("runs-on: ubuntu-latest", workflow) - self.assertEqual(workflow.count("runs-on: ubuntu-24.04"), 2) + self.assertEqual(workflow.count("runs-on: ubuntu-24.04"), 1) if __name__ == "__main__": From fb8138e6189e8a931fa937675f2c990421ba1dac Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sun, 13 Sep 2026 13:29:44 +0900 Subject: [PATCH 3/5] docs(sast-semgrep): describe the step-level guard in the job comment The comment still said the classifier verdict was consumed through `needs`; nothing consumes it that way any more. Co-Authored-By: Claude Fable 5.1 --- .github/workflows/sast-semgrep.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/sast-semgrep.yml b/.github/workflows/sast-semgrep.yml index 283eac0097..f8ab04b865 100644 --- a/.github/workflows/sast-semgrep.yml +++ b/.github/workflows/sast-semgrep.yml @@ -43,8 +43,9 @@ jobs: # The org ruleset IGNORES every `on:` filter (paths, branches, types) when it # runs this workflow in another repository, and a trigger-level skip would # leave `.github`'s classic required contexts Pending forever. Both - # mechanisms honour a JOB-level skip, so the doc/image-only decision is made - # here and consumed through `needs`. See + # mechanisms honour a job that runs and concludes on its own, so the + # doc/image-only decision is made by the classifier step below and consumed + # by the expensive steps' `if:` guards. See # docs/doctoring/required-workflow-path-filter-boundary.md. # Fails OPEN: an unreadable, empty, or truncated file list scans everything. # The gate lives inside this job as a step-level guard (one runner, not two). From 7e5b971a48dfdbd27d98ca8c39662c1d948935b4 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sun, 13 Sep 2026 17:16:09 +0900 Subject: [PATCH 4/5] 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) From 3452ed560573a2f4698be76f88db4081b9371465 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sun, 13 Sep 2026 19:46:31 +0900 Subject: [PATCH 5/5] fix(codeql): resolve SARIF rules from the referenced tool component `gather_findings` consulted only `tool.driver.rules`, so results whose rule lives in `tool.extensions` (every current CodeQL query pack: the real Python artifact has 0 driver rules and 43 extension rules) lost their security-severity and tags and passed the Medium+ gate fail-open (#2150). - `_component_rules`: honour `result.rule.toolComponent` (index | guid | name); absent means driver; unmatched means unresolved, never the wrong component. - `_rule_for_result`: resolve inside the referenced component only, so colliding rule ids stay distinct; validate `rule.index` against the declared id; `ruleId` != `rule.id`, bad indices, non-dict entries are unresolved. - `_finding_from_result`: an unresolved reference with no result-level score gates as `level=unresolved-rule` instead of silently passing. Driver-backed behaviour unchanged. Tests RED 8 -> GREEN 24; file coverage 100%. Full suite under coverage: 3062 passed / 1 skipped / 100% coverage; 13 timing-sensitive failures reproduced as passing in isolation (CPU contention). Closes #2150 Co-Authored-By: Claude Fable 5.1 --- scripts/ci/codeql_sarif_gate.py | 90 +++++++++++++++++----- tests/test_codeql_sarif_gate.py | 132 ++++++++++++++++++++++++++++++++ 2 files changed, 204 insertions(+), 18 deletions(-) diff --git a/scripts/ci/codeql_sarif_gate.py b/scripts/ci/codeql_sarif_gate.py index 3b232c3bdb..fb751ba90d 100644 --- a/scripts/ci/codeql_sarif_gate.py +++ b/scripts/ci/codeql_sarif_gate.py @@ -33,19 +33,66 @@ def iter_sarif_files(root: Path) -> list[Path]: return sorted(root.rglob("*.sarif")) -def _rule_for_result(result: dict[str, Any], rules: list[Any]) -> dict[str, Any]: - """Resolve the SARIF rule definition referenced by a result.""" - rules_by_id = { - str(rule.get("id") or ""): rule for rule in rules if isinstance(rule, dict) - } - rule = rules_by_id.get(str(result.get("ruleId") or ""), {}) - if rule: - return rule - rule_index = result.get("ruleIndex") - if isinstance(rule_index, int) and 0 <= rule_index < len(rules): - candidate = rules[rule_index] - if isinstance(candidate, dict): +UNRESOLVED_RULE_LEVEL = "unresolved-rule" + + +def _component_rules(result: dict[str, Any], tool: dict[str, Any]) -> list[Any] | None: + """Return the rules of the tool component a result references (SARIF 2.1.0 §3.54). + + No ``rule.toolComponent`` means the driver. Otherwise the reference selects one of + ``tool.extensions`` by ``index``, ``guid``, or ``name``; an unmatched reference + returns ``None`` so the caller can fail closed instead of consulting the wrong + component (issue #2150). + """ + reference = result.get("rule") if isinstance(result.get("rule"), dict) else {} + component_ref = reference.get("toolComponent") + if not isinstance(component_ref, dict): + return (tool.get("driver") or {}).get("rules") or [] + extensions = [ext for ext in tool.get("extensions") or [] if isinstance(ext, dict)] + index = component_ref.get("index") + if isinstance(index, int): + if 0 <= index < len(extensions): + return extensions[index].get("rules") or [] + return None + for key in ("guid", "name"): + wanted = component_ref.get(key) + if wanted is not None: + for extension in extensions: + if extension.get(key) == wanted: + return extension.get("rules") or [] + return None + return None + + +def _rule_for_result(result: dict[str, Any], tool: dict[str, Any]) -> dict[str, Any] | None: + """Resolve the SARIF rule definition a result references, or ``None`` if it cannot be. + + Resolution order inside the referenced component: ``rule.index`` (validated + against the declared id), then id lookup (``ruleId`` / ``rule.id``), then the + legacy ``ruleIndex``. Colliding ids across components stay distinct because + lookup never leaves the referenced component. + """ + rules = _component_rules(result, tool) + if rules is None: + return None + reference = result.get("rule") if isinstance(result.get("rule"), dict) else {} + declared_ids = {str(v) for v in (result.get("ruleId"), reference.get("id")) if v} + if len(declared_ids) > 1: + return None + declared_id = next(iter(declared_ids), "") + for index in (reference.get("index"), result.get("ruleIndex")): + if isinstance(index, int): + candidate = rules[index] if 0 <= index < len(rules) else None + if not isinstance(candidate, dict): + return None + if declared_id and str(candidate.get("id") or "") != declared_id: + return None return candidate + if declared_id: + for rule in rules: + if isinstance(rule, dict) and str(rule.get("id") or "") == declared_id: + return rule + return None return {} @@ -56,11 +103,16 @@ def _is_medium_plus(score: float | None, level: str, security_rule: bool) -> boo return security_rule and level in SEVERITY_LEVELS -def _finding_from_result(result: dict[str, Any], rules: list[Any]) -> Finding | None: - """Build a `Finding` for one SARIF result, or None if it doesn't gate the PR.""" +def _finding_from_result(result: dict[str, Any], tool: dict[str, Any]) -> Finding | None: + """Build a `Finding` for one SARIF result, or None if it doesn't gate the PR. + + A result whose rule reference cannot be resolved and that carries no explicit + security-severity gates as ``unresolved-rule`` rather than passing silently. + """ if not isinstance(result, dict) or result.get("suppressions"): return None - rule = _rule_for_result(result, rules) + resolved = _rule_for_result(result, tool) + rule = resolved or {} result_properties = result.get("properties") or {} rule_properties = rule.get("properties") or {} raw_score = result_properties.get("security-severity", rule_properties.get("security-severity")) @@ -71,7 +123,9 @@ def _finding_from_result(result: dict[str, Any], rules: list[Any]) -> Finding | level = str(result.get("level") or (rule.get("defaultConfiguration") or {}).get("level") or "none").lower() tags = {str(tag).lower() for tag in rule_properties.get("tags") or []} security_rule = "security" in tags or any(tag.startswith("external/cwe/") for tag in tags) - if not _is_medium_plus(score, level, security_rule): + if resolved is None and score is None: + level = UNRESOLVED_RULE_LEVEL + elif not _is_medium_plus(score, level, security_rule): return None physical = ((result.get("locations") or [{}])[0].get("physicalLocation") or {}) artifact = (physical.get("artifactLocation") or {}).get("uri") or "unknown" @@ -95,12 +149,12 @@ def gather_findings(root: Path) -> tuple[list[Finding], int, int]: for path in paths: payload = json.loads(path.read_text(encoding="utf-8")) for run in payload.get("runs") or []: - rules = ((run.get("tool") or {}).get("driver") or {}).get("rules") or [] + tool = run.get("tool") if isinstance(run.get("tool"), dict) else {} for result in run.get("results") or []: if not isinstance(result, dict): continue total_results += 1 - finding = _finding_from_result(result, rules) + finding = _finding_from_result(result, tool) if finding is not None: findings.append(finding) return findings, total_results, len(paths) diff --git a/tests/test_codeql_sarif_gate.py b/tests/test_codeql_sarif_gate.py index 186b9c80f1..1ab542dd89 100644 --- a/tests/test_codeql_sarif_gate.py +++ b/tests/test_codeql_sarif_gate.py @@ -203,3 +203,135 @@ def test_script_entrypoint_exits_with_main_status(tmp_path, monkeypatch): runpy.run_path(str(Path("scripts/ci/codeql_sarif_gate.py")), run_name="__main__") assert exc_info.value.code == 0 + + +def _extension_run(results: list[dict], *, driver_rules: list | None = None) -> dict: + """A run shaped like a real CodeQL artifact: 0 driver rules, rules in a query-pack extension.""" + extension_rules = [{"id": f"py/filler-{n}"} for n in range(17)] + [ + { + "id": "py/incomplete-url-substring-sanitization", + "properties": {"security-severity": "7.8", "tags": ["security", "external/cwe/cwe-020"]}, + "defaultConfiguration": {"level": "warning"}, + } + ] + return { + "tool": { + "driver": {"name": "CodeQL", "rules": driver_rules or []}, + "extensions": [{"name": "codeql/python-queries", "rules": extension_rules}], + }, + "results": results, + } + + +def test_gather_findings_resolves_rules_from_the_referenced_extension(tmp_path): + """Issue #2150: a result whose rule lives in tool.extensions must gate, not fail open.""" + _write_sarif( + tmp_path / "ext.sarif", + [ + _extension_run( + [ + { + "ruleId": "py/incomplete-url-substring-sanitization", + "rule": {"id": "py/incomplete-url-substring-sanitization", "index": 17, "toolComponent": {"index": 0}}, + "message": {"text": "doi check"}, + "locations": [{"physicalLocation": {"artifactLocation": {"uri": "src/x.py"}, "region": {"startLine": 4}}}], + }, + { + "ruleId": "py/incomplete-url-substring-sanitization", + "rule": {"index": 17, "toolComponent": {"name": "codeql/python-queries"}}, + "message": {"text": "by component name"}, + }, + ] + ) + ], + ) + + findings, total_results, _ = gate.gather_findings(tmp_path) + + assert total_results == 2 + assert [(f.rule_id, f.score, f.level, f.path, f.line) for f in findings] == [ + ("py/incomplete-url-substring-sanitization", 7.8, "warning", "src/x.py", 4), + ("py/incomplete-url-substring-sanitization", 7.8, "warning", "unknown", 0), + ] + + +def test_gather_findings_keeps_colliding_rule_ids_per_component(tmp_path): + """The same rule id in the driver and an extension resolves to the referenced component's metadata.""" + _write_sarif( + tmp_path / "collide.sarif", + [ + _extension_run( + [ + {"ruleId": "shared/id", "message": {"text": "driver copy"}}, + {"ruleId": "shared/id", "rule": {"toolComponent": {"index": 0}}, "message": {"text": "extension copy"}}, + ], + driver_rules=[{"id": "shared/id", "defaultConfiguration": {"level": "note"}}], + ) + ], + ) + # extension gets a colliding scored rule appended + payload = json.loads((tmp_path / "collide.sarif").read_text(encoding="utf-8")) + payload["runs"][0]["tool"]["extensions"][0]["rules"].append( + {"id": "shared/id", "properties": {"security-severity": "9.1"}} + ) + (tmp_path / "collide.sarif").write_text(json.dumps(payload), encoding="utf-8") + + findings, _, _ = gate.gather_findings(tmp_path) + + assert [(f.message, f.score) for f in findings] == [("extension copy", 9.1)] + + +@pytest.mark.parametrize( + "result", + [ + {"ruleId": "py/x", "rule": {"index": 17, "toolComponent": {"index": 5}}}, + {"ruleId": "py/x", "rule": {"index": 17, "toolComponent": {"name": "codeql/no-such-pack"}}}, + {"ruleId": "py/x", "rule": {"index": 99, "toolComponent": {"index": 0}}}, + {"ruleId": "py/other", "rule": {"index": 17, "toolComponent": {"index": 0}}}, + {"ruleId": "py/x", "rule": {"id": "py/y", "toolComponent": {"index": 0}}}, + {"rule": {"index": 3, "toolComponent": {"guid": "00000000-0000-0000-0000-000000000000"}}}, + {"ruleId": "py/x", "rule": {"toolComponent": {}}}, + ], + ids=["bad-component-index", "bad-component-name", "bad-rule-index", "indexed-rule-id-mismatch", "ruleId-vs-rule-id-mismatch", "bad-component-guid", "empty-component-reference"], +) +def test_gather_findings_fails_closed_on_unresolvable_rule_references(tmp_path, result): + """A rule reference that cannot be resolved, with no severity evidence, gates instead of passing.""" + _write_sarif(tmp_path / "bad.sarif", [_extension_run([dict(result, message={"text": "m"})])]) + + findings, _, _ = gate.gather_findings(tmp_path) + + assert len(findings) == 1 + assert findings[0].level == "unresolved-rule" + assert findings[0].score is None + assert gate.format_finding(findings[0]).startswith("CODEQL_FINDING rule=") + + +def test_gather_findings_uses_result_score_even_when_rule_is_unresolvable(tmp_path): + """Explicit result-level security-severity still decides gating when the rule cannot be resolved.""" + _write_sarif( + tmp_path / "scored.sarif", + [_extension_run([{"ruleId": "py/x", "rule": {"toolComponent": {"index": 9}}, "properties": {"security-severity": "1.0"}}])], + ) + + findings, _, _ = gate.gather_findings(tmp_path) + + assert findings == [] + + +def test_gather_findings_gates_an_unreferenced_result_on_its_own_score(tmp_path): + """A result with no rule reference at all is judged purely on its result-level severity.""" + _write_sarif(tmp_path / "bare.sarif", [_extension_run([{"properties": {"security-severity": "6.0"}}])]) + + findings, _, _ = gate.gather_findings(tmp_path) + + assert [(f.rule_id, f.score, f.level) for f in findings] == [("unknown", 6.0, "none")] + + +def test_gather_findings_leaves_resolved_non_security_extension_rules_alone(tmp_path): + """A resolved extension rule with no security metadata keeps the existing non-gating semantics.""" + _write_sarif( + tmp_path / "style.sarif", + [_extension_run([{"rule": {"index": 3, "toolComponent": {"index": 0}}, "level": "note", "message": {"text": "style"}}])], + ) + + assert gate.gather_findings(tmp_path)[0] == []