diff --git a/.github/workflows/security-scan.yml b/.github/workflows/security-scan.yml index 29376269d..f16049acc 100644 --- a/.github/workflows/security-scan.yml +++ b/.github/workflows/security-scan.yml @@ -74,6 +74,10 @@ jobs: path: source fetch-depth: 0 persist-credentials: false + - name: Discard checkout-provided OSV result files + run: | + set -euo pipefail + rm -rf -- source/old-results.json source/new-results.json - name: Scan base with OSV id: osv_base continue-on-error: true @@ -105,6 +109,23 @@ jobs: --allow-no-lockfiles -r source/ + - name: Preserve base OSV output outside the checkout path + run: | + set -euo pipefail + dest="${RUNNER_TEMP}/osv-old-results.json" + rm -f "${dest}" + umask 077 + for candidate in old-results.json source/old-results.json; do + if [ -f "${candidate}" ] && [ ! -L "${candidate}" ] && [ -s "${candidate}" ]; then + /usr/bin/sudo --non-interactive /usr/bin/cat -- "${candidate}" | /usr/bin/tee "${dest}" >/dev/null + test -s "${dest}" + test -O "${dest}" + echo "Preserved OSV base output from ${candidate}" + exit 0 + fi + done + echo "::error::OSV base output was not present after the base scan steps." + exit 1 - name: Checkout head uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: @@ -113,6 +134,10 @@ jobs: path: source fetch-depth: 0 persist-credentials: false + - name: Discard checkout-provided OSV result files + run: | + set -euo pipefail + rm -rf -- source/old-results.json source/new-results.json - name: Scan head with OSV id: osv_head continue-on-error: true @@ -144,11 +169,33 @@ jobs: --allow-no-lockfiles -r source/ + - name: Capture head OSV output outside the checkout path + run: | + set -euo pipefail + dest="${RUNNER_TEMP}/osv-new-results.json" + rm -f "${dest}" + umask 077 + for candidate in new-results.json source/new-results.json; do + if [ -f "${candidate}" ] && [ ! -L "${candidate}" ] && [ -s "${candidate}" ]; then + /usr/bin/sudo --non-interactive /usr/bin/cat -- "${candidate}" | /usr/bin/tee "${dest}" >/dev/null + test -s "${dest}" + test -O "${dest}" + echo "Captured OSV head output from ${candidate}" + exit 0 + fi + done + echo "::error::OSV head output was not present after the head scan steps." + exit 1 - name: Require OSV scan output run: | set -euo pipefail - test -s old-results.json - test -s new-results.json + old="${RUNNER_TEMP}/osv-old-results.json" + new="${RUNNER_TEMP}/osv-new-results.json" + test -s "${old}" + test -s "${new}" + rm -f old-results.json new-results.json + cp "${old}" old-results.json + cp "${new}" new-results.json - name: Print OSV findings being compared shell: python3 {0} run: | @@ -245,8 +292,8 @@ jobs: with: name: osv-scan-debug path: | - old-results.json - new-results.json + ${{ runner.temp }}/osv-old-results.json + ${{ runner.temp }}/osv-new-results.json results.sarif if-no-files-found: ignore retention-days: 5 diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b0ef8d44..93cd9e92e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,14 @@ Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Preserve OSV scan output in private, runner-owned `RUNNER_TEMP` files before a + fork head checkout, using a bounded privileged read so root-owned mode-`0600` + scanner output does not depend on world readability; keep the base capture + external during the head scan, materialize reporter inputs exactly once after + unlinking root-owned workspace files, discard checkout-provided result files, + links, or directories before each scan, upload runner-owned captures for + failure diagnostics, and never treat post-checkout `source/*.json` as + reporter input. - Honor each trusted base project's exact, integrity-bearing pnpm `packageManager` specification in OpenCode coverage images through the pinned Node distribution's Corepack runtime, instead of admitting the specification diff --git a/docs/doctoring/osv-cross-fork-result-isolation.md b/docs/doctoring/osv-cross-fork-result-isolation.md index c9a6c0a98..191524825 100644 --- a/docs/doctoring/osv-cross-fork-result-isolation.md +++ b/docs/doctoring/osv-cross-fork-result-isolation.md @@ -8,24 +8,57 @@ checkout used the upstream repository while the head belonged to a fork. `actions/checkout` detected the different repository origin and replaced the workspace before checking out the fork, including the untracked base result. `clean: false` does not preserve files when checkout must replace a workspace -whose repository identity changed. +whose repository identity changed. Copying a captured result back onto the +workspace `old-results.json` can also fail: the scanner action may create that +file as root, so a runner-user overwrite returns permission denied. +The inverse copy has the same unowned boundary: a runner-user `cp` can read a +root-owned scanner result only while the container happens to leave it +world-readable. A mode such as `0600` makes that capture fail before the +checkout-isolation guarantee can be established. ## Decision Checkout both exact repositories into the same `source/` child directory and -scan that directory. The head checkout may replace `source/` when the repository -identity changes, but the base result remains at the workspace root. Reusing the -same checkout path also gives the base and head scan identical source paths, so -the existing OSV comparison retains its meaning. Missing or empty output remains -a hard failure; this change does not weaken vulnerability comparison. +scan that directory. Copy a non-empty scanner result into +`${RUNNER_TEMP}/osv-old-results.json` and `${RUNNER_TEMP}/osv-new-results.json` +immediately after each scan. Do not copy those captures back onto an existing +workspace `old-results.json`: the OSV action may create that file as root, and +a runner-user `cp` then fails with permission denied (observed on +ContextualWisdomLab/.github#1257). For the scanner-to-runner transfer, accept +only a fixed-path, non-empty regular file that is not a symbolic link, use the +GitHub-hosted Linux runner's passwordless `/usr/bin/sudo` solely for a fixed +`/usr/bin/cat`, and let the unprivileged shell create the destination after +`umask 077`. The resulting `RUNNER_TEMP` capture is therefore runner-owned and +mode `0600`; the workflow verifies both ownership and non-empty content. It +does not `chmod` the scanner output, grant other users read access, or make the +reporter privileged. Keep the base capture outside the workspace throughout +the head scan; no consumer needs a pre-scan workspace copy. Materialize both +reporter inputs exactly once by unlinking the scanner-created workspace files +and copying from `RUNNER_TEMP`. Discard `source/old-results.json` and +`source/new-results.json` as exact paths before each scan, whether a fork plants +a file, link, or directory there, so the planted entry cannot abort the scan or +become reporter input. After the head checkout, never treat checkout-path JSON +as scanner output. Missing or empty captured output remains a hard failure; a +zero-finding head scan does not skip the base comparison. This change does not +weaken vulnerability comparison. The always-run debug upload reads the private +runner captures directly rather than root-owned workspace results, so an early +failure does not replace the primary diagnostic with an artifact permission +error. ## Verification and rollback - The workflow contract proves both checkouts target `source/`, every scan reads - that same directory, and both result files remain at the workspace root. + that same directory, captures land in `RUNNER_TEMP` before compare, reporter + materialization unlinks before copy, and post-checkout `source/*.json` is not + reporter input. +- The executable regression runs both production capture steps against an + unreadable scanner result through a bounded privilege stand-in, then proves + the capture contains the exact result and is runner-owned with mode `0600`. +- The artifact contract uploads the runner-owned captures, including on failure, + and never asks the uploader to read root-owned workspace result files. - `actionlint` validates the edited workflow. -- Rerun a fork PR's `Security Scan`; both `old-results.json` and - `new-results.json` must be non-empty before the reporter runs. +- Rerun a fork PR's `Security Scan`; both captured result files must be + non-empty before the reporter runs. - Roll back only after another job-scoped store retains the base artifact across repository replacement without changing the compared source paths. @@ -34,6 +67,10 @@ a hard failure; this change does not weaken vulnerability comparison. GitHub. (2026). *Variables reference*. GitHub Docs. Retrieved August 22, 2026, from https://docs.github.com/en/actions/reference/variables-reference +GitHub. (2026). *GitHub-hosted runners reference*. GitHub Docs. Retrieved +August 23, 2026, from +https://docs.github.com/en/actions/reference/runners/github-hosted-runners + GitHub Actions. (2026). *Checkout*. GitHub. Retrieved August 22, 2026, from https://github.com/actions/checkout diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index e58f5e6c0..9378d7474 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -4,6 +4,7 @@ import os import shlex import shutil +import stat import subprocess import sys import textwrap @@ -1257,17 +1258,142 @@ def test_security_scan_skips_dependency_review_when_dependency_graph_is_unavaila def test_security_scan_preserves_base_output_across_cross_fork_checkout() -> None: - """Limit cross-fork replacement to a child checkout directory.""" + """Keep captures external until the reporter materializes its inputs.""" workflow = workflow_text("security-scan.yml") 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("source/") == 4 + assert workflow.count("-r\n source/") == 4 assert "clean: false" not in workflow - assert "test -s old-results.json" in workflow - assert "test -s new-results.json" in workflow + assert "Preserve base OSV output outside the checkout path" in workflow + assert "Restore preserved base OSV output" not in workflow + assert "Capture head OSV output outside the checkout path" in workflow + assert "${RUNNER_TEMP}/osv-old-results.json" in workflow + assert "${RUNNER_TEMP}/osv-new-results.json" in workflow + assert "source/old-results.json" in workflow + assert "source/new-results.json" in workflow + preserve = workflow.index( + " - name: Preserve base OSV output outside the checkout path" + ) + checkout_head = workflow.index(" - name: Checkout head") + discard_after_checkout = workflow.index( + " - name: Discard checkout-provided OSV result files", checkout_head + ) + scan_head = workflow.index(" - name: Scan head with OSV") + capture_head = workflow.index( + " - name: Capture head OSV output outside the checkout path" + ) + require_output = workflow.index(" - name: Require OSV scan output") + assert ( + preserve + < checkout_head + < discard_after_checkout + < scan_head + < capture_head + < require_output + ) + require_block = workflow[ + require_output : workflow.index(" - name: Print OSV findings being compared") + ] + debug_upload = workflow_step(workflow, "Upload OSV debug artifacts") + assert "source/old-results.json" not in require_block + assert "source/new-results.json" not in require_block + assert 'cp "${dest}" "${GITHUB_WORKSPACE}/old-results.json"' not in workflow + assert 'cp "${src}" "${GITHUB_WORKSPACE}/old-results.json"' not in workflow + assert ( + workflow.count( + "rm -rf -- source/old-results.json source/new-results.json" + ) + == 2 + ) + assert 'test -s "${old}"' in require_block + assert 'test -s "${new}"' in require_block + assert "${{ runner.temp }}/osv-old-results.json" in debug_upload + assert "${{ runner.temp }}/osv-new-results.json" in debug_upload + assert "\n old-results.json\n" not in debug_upload + assert "\n new-results.json\n" not in debug_upload + + +@pytest.mark.parametrize( + ("step_name", "candidate_name", "destination_name"), + [ + ( + "Preserve base OSV output outside the checkout path", + "old-results.json", + "osv-old-results.json", + ), + ( + "Preserve base OSV output outside the checkout path", + "source/old-results.json", + "osv-old-results.json", + ), + ( + "Capture head OSV output outside the checkout path", + "source/new-results.json", + "osv-new-results.json", + ), + ], +) +def test_security_scan_transfers_unreadable_scanner_output_to_runner( + tmp_path: Path, + step_name: str, + candidate_name: str, + destination_name: str, +) -> None: + """A privileged read must create a private runner-owned capture.""" + workflow = workflow_text("security-scan.yml") + run_marker = " run: |\n" + step = workflow_step(workflow, step_name) + assert run_marker in step + + fake_sudo = tmp_path / "sudo" + fake_sudo.write_text( + """#!/bin/sh +set -eu +test "$1" = "--non-interactive" +shift +test "$1" = "/usr/bin/cat" +shift +test "$1" = "--" +shift +chmod u+r "$1" +exec /bin/cat "$1" +""", + encoding="utf-8", + ) + fake_sudo.chmod(0o700) + + source = tmp_path / candidate_name + source.parent.mkdir(parents=True, exist_ok=True) + expected = '{"results": []}\n' + source.write_text(expected, encoding="utf-8") + source.chmod(0) + runner_temp = tmp_path / "runner-temp" + runner_temp.mkdir() + + script = textwrap.dedent(step.split(run_marker, 1)[1]).replace( + "/usr/bin/sudo", shlex.quote(str(fake_sudo)) + ) + result = subprocess.run( + ["/bin/bash", "-c", script], + cwd=tmp_path, + env={ + **os.environ, + "GITHUB_WORKSPACE": str(tmp_path), + "RUNNER_TEMP": str(runner_temp), + }, + check=False, + capture_output=True, + text=True, + ) + + destination = runner_temp / destination_name + assert result.returncode == 0, result.stderr + assert destination.read_text(encoding="utf-8") == expected + assert destination.stat().st_uid == os.geteuid() + assert stat.S_IMODE(destination.stat().st_mode) == 0o600 def test_secret_scan_push_limits_gitleaks_to_current_branch_history() -> None: