diff --git a/.github/workflows/dependency-review.yml b/.github/workflows/dependency-review.yml index f8c4b6278..af840dbe4 100644 --- a/.github/workflows/dependency-review.yml +++ b/.github/workflows/dependency-review.yml @@ -84,6 +84,8 @@ jobs: } def is_dependency_input(path: PurePosixPath) -> bool: + # Workflow code changes are covered by CI tests; they do not + # change a dependency graph and should not force a comparison. name = path.name return ( name in exact_names @@ -96,7 +98,6 @@ jobs: or path.as_posix() in { ".github/dependabot.yaml", ".github/dependabot.yml", - ".github/workflows/dependency-review.yml", "vendor/modules.txt", } ) @@ -145,9 +146,6 @@ jobs: if root_valid and all(covered(path) for path in changed if PurePosixPath(path).name == "package.json"): snapshot_detector = "NpmLockfile3" - elif changed == [".github/workflows/dependency-review.yml"]: - # Exercise both focused detectors on the workflow change itself. - snapshot_detector = "UvLock,NpmLockfile3" with open(os.environ["GITHUB_OUTPUT"], "a", encoding="utf-8") as handle: handle.write(f"changed={'true' if changed else 'false'}\n") handle.write(f"count={len(changed)}\n") @@ -158,8 +156,8 @@ jobs: handle.writelines(f"- `{path}`\n" for path in changed) else: handle.write( - "No dependency manifests, lockfiles, or dependency-review " - "configuration changed between the exact base and head trees.\n" + "No dependency inputs changed between the exact base " + "and head trees.\n" ) PY @@ -193,15 +191,7 @@ jobs: locks = [Path(raw.decode("utf-8")) for raw in delta.split(b"\0") if raw and Path(raw.decode("utf-8")).name == "package-lock.json"] else: - # The workflow-only PR exercises both lockfile ecosystems. - locks = {Path("uv.lock")} - for sha in (os.environ["BASE_SHA"], os.environ["HEAD_SHA"]): - tree = subprocess.run( - ["git", "ls-tree", "-r", "--name-only", "-z", sha], - check=True, capture_output=True, - ).stdout - locks.update(Path(raw.decode("utf-8")) for raw in tree.split(b"\0") if raw - and Path(raw.decode("utf-8")).name == "package-lock.json") + raise ValueError(f"Unexpected focused snapshot detector: {detector}") for sha, directory in ( (os.environ["BASE_SHA"], os.environ["SNAPSHOT_BASE_SOURCE"]), @@ -352,8 +342,9 @@ jobs: pkg:npm/@img/sharp-win32-x64 comment-summary-in-pr: always - # Preserve the graph comparison even when a composite license is rejected. - # An empty/missing action output stays empty; it is not a clean result. + # Fetch the same dependency diff from GitHub's API instead of expanding + # dependency-changes in an Actions expression. Large grouped updates can + # exceed the runner's template-object limit before the record step starts. - name: Record dependency review evidence if: >- ${{ always() && github.event_name == 'pull_request' && @@ -362,41 +353,8 @@ jobs: BASE_SHA: ${{ github.event.pull_request.base.sha }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} REVIEW_OUTCOME: ${{ steps.review.outcome }} - DEPENDENCY_CHANGES: ${{ steps.review.outputs.dependency-changes }} - INVALID_LICENSE_CHANGES: ${{ steps.review.outputs.invalid-license-changes }} - VULNERABLE_CHANGES: ${{ steps.review.outputs.vulnerable-changes }} - DENIED_CHANGES: ${{ steps.review.outputs.denied-changes }} - run: | - python3 - <<'PY' - import json - import os - from pathlib import Path - - evidence = { - "schema_version": 1, - "base_sha": os.environ["BASE_SHA"], - "head_sha": os.environ["HEAD_SHA"], - "run_id": os.environ["GITHUB_RUN_ID"], - "run_attempt": os.environ["GITHUB_RUN_ATTEMPT"], - "review_outcome": os.environ["REVIEW_OUTCOME"], - # Retain the exact raw JSON strings, including duplicate entries, - # full SPDX expressions, and empty outputs from incomplete reviews. - "raw_outputs": { - name: os.environ[variable] - for name, variable in { - "dependency-changes": "DEPENDENCY_CHANGES", - "invalid-license-changes": "INVALID_LICENSE_CHANGES", - "vulnerable-changes": "VULNERABLE_CHANGES", - "denied-changes": "DENIED_CHANGES", - }.items() - }, - } - destination = Path("dependency-review-evidence") - destination.mkdir(exist_ok=True) - (destination / "review.json").write_text( - json.dumps(evidence, indent=2) + "\n", encoding="utf-8" - ) - PY + GITHUB_TOKEN: ${{ github.token }} + run: python3 scripts/ci/record_dependency_review.py - name: Upload dependency review evidence if: >- @@ -405,7 +363,7 @@ jobs: uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: dependency-review-${{ github.event.pull_request.head.sha }}-${{ github.run_attempt }} - path: dependency-review-evidence/review.json + path: dependency-review-evidence/ if-no-files-found: error retention-days: 30 diff --git a/scripts/ci/record_dependency_review.py b/scripts/ci/record_dependency_review.py new file mode 100644 index 000000000..96b6b080a --- /dev/null +++ b/scripts/ci/record_dependency_review.py @@ -0,0 +1,153 @@ +"""Save the complete GitHub dependency diff without Actions output expansion.""" + +from __future__ import annotations + +import hashlib +import json +import os +import re +import subprocess +from pathlib import Path +from urllib.request import Request, urlopen + + +def _changed_files(base: str, head: str) -> set[str]: + output = subprocess.run( + ["git", "diff", "--name-only", "--no-renames", "-z", base, head], + check=True, + capture_output=True, + ).stdout + return {os.fsdecode(path) for path in output.split(b"\0") if path} + + +def _npm_lock_components(sha: str, manifest: str) -> set[tuple[str, str, str]]: + result = subprocess.run(["git", "show", f"{sha}:{manifest}"], capture_output=True) + if result.returncode: + # A newly added or deleted lockfile is absent on one side of the diff. + return set() + lock = json.loads(result.stdout) + packages = lock.get("packages") + if not isinstance(packages, dict): + raise ValueError(f"Invalid npm lockfile in {sha}:{manifest}") + components = set() + for location, package in packages.items(): + if not isinstance(location, str) or not isinstance(package, dict): + raise ValueError(f"Invalid npm lockfile entry in {sha}:{manifest}") + name = package.get("name") + if not name and "node_modules/" in location: + name = location.rsplit("node_modules/", 1)[1] + version = package.get("version") + if isinstance(name, str) and isinstance(version, str): + scope = "development" if package.get("dev") is True else "runtime" + components.add((name, version, scope)) + return components + + +def _validate_changes( + changes: list[dict], base: str, head: str, changed_files: set[str] +) -> None: + npm_locks: dict[tuple[str, str], set[tuple[str, str, str]]] = {} + for change in changes: + manifest = change.get("manifest") + if not isinstance(manifest, str) or manifest not in changed_files: + raise ValueError( + "Dependency graph includes a change in an unchanged manifest: " + f"{manifest!r}" + ) + if change.get("ecosystem") != "npm" or not manifest.endswith( + "package-lock.json" + ): + continue + name, version, scope = ( + change.get("name"), + change.get("version"), + change.get("scope"), + ) + if not all(isinstance(value, str) for value in (name, version, scope)): + raise ValueError("npm dependency change lacks name, version, or scope") + for sha in (base, head): + if (sha, manifest) not in npm_locks: + npm_locks[(sha, manifest)] = _npm_lock_components(sha, manifest) + component = (name, version, scope) + on_base = component in npm_locks[(base, manifest)] + on_head = component in npm_locks[(head, manifest)] + kind = change.get("change_type") + if kind == "added" and on_head and not on_base: + continue + if kind == "removed" and on_base and not on_head: + continue + raise ValueError( + "npm dependency change does not match lockfile version/scope: " + f"{manifest} {name}@{version} {scope} {kind}" + ) + + +def record_dependency_review( + environment: dict[str, str], + destination: Path, + opener=urlopen, + changed_files: set[str] | None = None, +) -> dict: + base = environment["BASE_SHA"] + head = environment["HEAD_SHA"] + if not all(re.fullmatch(r"[0-9a-f]{40}", sha) for sha in (base, head)): + raise ValueError("Dependency review requires exact commit SHAs") + + repository = environment["GITHUB_REPOSITORY"] + if not re.fullmatch(r"[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+", repository): + raise ValueError("Invalid GitHub repository") + + api_root = environment.get("GITHUB_API_URL", "https://api.github.com").rstrip("/") + url = f"{api_root}/repos/{repository}/dependency-graph/compare/{base}...{head}" + request = Request( + url, + headers={ + "Accept": "application/vnd.github+json", + "Authorization": f"Bearer {environment['GITHUB_TOKEN']}", + "X-GitHub-Api-Version": "2022-11-28", + }, + ) + with opener(request, timeout=90) as response: + raw_diff = response.read() + snapshot_warning = response.headers.get( + "x-github-dependency-graph-snapshot-warnings" + ) + if snapshot_warning: + raise ValueError( + "Dependency graph reported snapshot warnings; refusing to save " + "an incomplete comparison as review evidence" + ) + changes = json.loads(raw_diff) + if not isinstance(changes, list) or not all( + isinstance(item, dict) for item in changes + ): + raise ValueError("Dependency diff API did not return a complete change list") + if changes: + _validate_changes( + changes, + base, + head, + changed_files if changed_files is not None else _changed_files(base, head), + ) + + destination.mkdir(parents=True, exist_ok=True) + (destination / "dependency-diff.json").write_bytes(raw_diff) + evidence = { + "schema_version": 2, + "base_sha": base, + "head_sha": head, + "run_id": environment["GITHUB_RUN_ID"], + "run_attempt": environment["GITHUB_RUN_ATTEMPT"], + "review_outcome": environment["REVIEW_OUTCOME"], + "dependency_diff_source": url, + "dependency_diff_count": len(changes), + "dependency_diff_sha256": hashlib.sha256(raw_diff).hexdigest(), + "dependency_diff_file": "dependency-diff.json", + } + (destination / "review.json").write_text(json.dumps(evidence, indent=2) + "\n") + return evidence + + +if __name__ == "__main__": + result = record_dependency_review(os.environ, Path("dependency-review-evidence")) + print(f"Saved {result['dependency_diff_count']} dependency changes") diff --git a/tests/unit/test_dependency_review_workflow.py b/tests/unit/test_dependency_review_workflow.py index 978a9ae50..70376021f 100644 --- a/tests/unit/test_dependency_review_workflow.py +++ b/tests/unit/test_dependency_review_workflow.py @@ -1,9 +1,11 @@ from __future__ import annotations +import hashlib +import io import json import os +import runpy import subprocess -import sys from pathlib import Path import pytest @@ -60,7 +62,6 @@ def test_dependency_review_submits_pr_head_snapshot_and_retries_warnings() -> No "vendor/modules.txt", "Dockerfile", ".github/dependabot.yaml", - ".github/workflows/dependency-review.yml", ): assert protected_input in detection_script assert '"git", "diff", "--name-only", "-z", base, head' in detection_script @@ -144,7 +145,7 @@ def test_dependency_review_submits_pr_head_snapshot_and_retries_warnings() -> No ), (["requirements.txt"], "true", ""), (["uv.lock", "requirements.txt"], "true", ""), - ([".github/workflows/dependency-review.yml"], "true", "UvLock,NpmLockfile3"), + ([".github/workflows/dependency-review.yml"], "false", ""), (["src/app.py"], "false", ""), ], ) @@ -391,11 +392,9 @@ def test_numpy_remains_subject_to_the_unchanged_license_policy() -> None: ) -@pytest.mark.parametrize( - "review_outcome,raw_output", [("failure", None), ("success", "")] -) -def test_dependency_review_evidence_preserves_composite_license_and_unknown_outputs( - tmp_path: Path, review_outcome: str, raw_output: str | None +@pytest.mark.parametrize("review_outcome", ["failure", "success"]) +def test_dependency_review_evidence_preserves_large_complete_api_diff( + tmp_path: Path, review_outcome: str ) -> None: steps = _load_workflow()["jobs"]["dependency-review"]["steps"] review = next(step for step in steps if step["name"] == "Dependency Review") @@ -410,47 +409,282 @@ def test_dependency_review_evidence_preserves_composite_license_and_unknown_outp assert "always()" in step["if"] assert "github.event_name == 'pull_request'" in step["if"] assert "steps.dependency-changes.outputs.changed == 'true'" in step["if"] - assert record["env"]["INVALID_LICENSE_CHANGES"] == ( - "${{ steps.review.outputs.invalid-license-changes }}" - ) + assert record["run"] == "python3 scripts/ci/record_dependency_review.py" + assert not any("steps.review.outputs" in value for value in record["env"].values()) assert "github.event.pull_request.head.sha" in upload["with"]["name"] - assert upload["with"]["path"] == "dependency-review-evidence/review.json" + assert upload["with"]["path"] == "dependency-review-evidence/" assert upload["with"]["if-no-files-found"] == "error" expression = "BSD-3-Clause AND 0BSD AND MIT AND Zlib AND CC0-1.0" - finding = {"name": "numpy", "version": "2.5.4", "license": expression} - # Keep duplicated occurrences, the full AND expression, and hostile text as data. - invalid = json.dumps( - {"forbidden": [finding, finding], "unresolved": [], "unlicensed": []} - ) - dependencies = json.dumps([finding, {"name": "$(touch injected)"}]) + finding = { + "name": "numpy", + "version": "2.5.4", + "license": expression, + "manifest": "requirements.txt", + "ecosystem": "pip", + } + # This response exceeds the Actions template-object limit hit by grouped PRs. + changes = [finding, finding, {**finding, "name": "$(touch injected)"}] + [ + { + **finding, + "name": f"package-{index}", + "license": "MIT", + "version": "1.0.0", + } + for index in range(1500) + ] + raw = json.dumps(changes).encode() + assert len(raw) > 64_000 environment = { - # A failing subprocess may print its environment in pytest's traceback. - # Pass only this test's inputs, never credentials from the caller. - "PATH": os.pathsep.join((str(Path(sys.executable).parent), os.defpath)), "BASE_SHA": "a" * 40, "HEAD_SHA": "b" * 40, + "GITHUB_REPOSITORY": "groupthinking/EventRelay", + "GITHUB_API_URL": "https://api.github.com", + "GITHUB_TOKEN": "dummy-test-token", "GITHUB_RUN_ID": "123", "GITHUB_RUN_ATTEMPT": "2", "REVIEW_OUTCOME": review_outcome, - "DEPENDENCY_CHANGES": dependencies if raw_output is None else raw_output, - "INVALID_LICENSE_CHANGES": invalid if raw_output is None else raw_output, - "VULNERABLE_CHANGES": "[]" if raw_output is None else raw_output, - "DENIED_CHANGES": "", } - subprocess.run( - ["bash", "-e", "-c", record["run"]], cwd=tmp_path, env=environment, check=True + module = runpy.run_path( + str(WORKFLOW_PATH.parents[2] / "scripts/ci/record_dependency_review.py") + ) + seen_requests = [] + + class ApiResponse(io.BytesIO): + headers: dict[str, str] = {} + + def fetch(request, timeout): + seen_requests.append((request, timeout)) + return ApiResponse(raw) + + destination = tmp_path / "dependency-review-evidence" + module["record_dependency_review"]( + environment, destination, opener=fetch, changed_files={"requirements.txt"} ) - saved = json.loads((tmp_path / upload["with"]["path"]).read_text()) + saved = json.loads((destination / "review.json").read_text()) assert saved["base_sha"] == "a" * 40 assert saved["head_sha"] == "b" * 40 assert saved["run_id"] == "123" assert saved["run_attempt"] == "2" assert saved["review_outcome"] == review_outcome - assert saved["raw_outputs"] == { - "dependency-changes": environment["DEPENDENCY_CHANGES"], - "invalid-license-changes": environment["INVALID_LICENSE_CHANGES"], - "vulnerable-changes": environment["VULNERABLE_CHANGES"], - "denied-changes": "", - } + assert saved["dependency_diff_count"] == len(changes) + assert saved["dependency_diff_sha256"] == hashlib.sha256(raw).hexdigest() + assert (destination / saved["dependency_diff_file"]).read_bytes() == raw + assert saved["dependency_diff_source"] == seen_requests[0][0].full_url + assert seen_requests[0][1] == 90 + assert seen_requests[0][0].get_header("Authorization") == "Bearer dummy-test-token" assert not (tmp_path / "injected").exists() + + +@pytest.mark.parametrize("raw", [b"[]", b'[{"name":"partial"}]']) +def test_dependency_review_evidence_rejects_snapshot_warnings_before_writing( + tmp_path: Path, raw: bytes +) -> None: + module = runpy.run_path( + str(WORKFLOW_PATH.parents[2] / "scripts/ci/record_dependency_review.py") + ) + environment = { + "BASE_SHA": "a" * 40, + "HEAD_SHA": "b" * 40, + "GITHUB_REPOSITORY": "groupthinking/EventRelay", + "GITHUB_TOKEN": "dummy-test-token", + "GITHUB_RUN_ID": "123", + "GITHUB_RUN_ATTEMPT": "1", + "REVIEW_OUTCOME": "success", + } + + class ApiResponse(io.BytesIO): + headers = { + "x-github-dependency-graph-snapshot-warnings": "bWlzc2luZyBzbmFwc2hvdHM=" + } + + destination = tmp_path / "dependency-review-evidence" + with pytest.raises(ValueError, match="snapshot warnings"): + module["record_dependency_review"]( + environment, destination, opener=lambda request, timeout: ApiResponse(raw) + ) + assert not destination.exists() + + +def test_dependency_review_evidence_saves_warning_free_empty_diff( + tmp_path: Path, +) -> None: + module = runpy.run_path( + str(WORKFLOW_PATH.parents[2] / "scripts/ci/record_dependency_review.py") + ) + environment = { + "BASE_SHA": "a" * 40, + "HEAD_SHA": "b" * 40, + "GITHUB_REPOSITORY": "groupthinking/EventRelay", + "GITHUB_TOKEN": "dummy-test-token", + "GITHUB_RUN_ID": "123", + "GITHUB_RUN_ATTEMPT": "1", + "REVIEW_OUTCOME": "success", + } + + class ApiResponse(io.BytesIO): + headers: dict[str, str] = {} + + destination = tmp_path / "dependency-review-evidence" + saved = module["record_dependency_review"]( + environment, destination, opener=lambda request, timeout: ApiResponse(b"[]") + ) + assert saved["dependency_diff_count"] == 0 + assert (destination / "dependency-diff.json").read_bytes() == b"[]" + + +def test_dependency_review_rejects_warning_free_graph_drift( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + repo = tmp_path / "repo" + repo.mkdir() + subprocess.run(["git", "init", "-q"], cwd=repo, check=True) + subprocess.run(["git", "config", "user.name", "Test"], cwd=repo, check=True) + subprocess.run( + ["git", "config", "user.email", "test@example.com"], cwd=repo, check=True + ) + + def commit_lock(version: str) -> str: + lock = { + "lockfileVersion": 3, + "packages": { + "": {"name": "eventrelay", "version": "1.0.0"}, + "node_modules/fast-uri": {"version": version, "dev": True}, + "node_modules/hono": {"version": "4.13.7", "dev": True}, + }, + } + (repo / "package-lock.json").write_text(json.dumps(lock) + "\n") + subprocess.run(["git", "add", "package-lock.json"], cwd=repo, check=True) + subprocess.run(["git", "commit", "-qm", version], cwd=repo, check=True) + return subprocess.check_output( + ["git", "rev-parse", "HEAD"], cwd=repo, text=True + ).strip() + + base = commit_lock("3.1.7") + head = commit_lock("3.1.9") + monkeypatch.chdir(repo) + module = runpy.run_path( + str(WORKFLOW_PATH.parents[2] / "scripts/ci/record_dependency_review.py") + ) + environment = { + "BASE_SHA": base, + "HEAD_SHA": head, + "GITHUB_REPOSITORY": "groupthinking/EventRelay", + "GITHUB_TOKEN": "dummy-test-token", + "GITHUB_RUN_ID": "123", + "GITHUB_RUN_ATTEMPT": "1", + "REVIEW_OUTCOME": "success", + } + added = { + "change_type": "added", + "manifest": "package-lock.json", + "ecosystem": "npm", + "name": "fast-uri", + "version": "3.1.9", + "scope": "development", + } + removed = {**added, "change_type": "removed", "version": "3.1.7"} + + class ApiResponse(io.BytesIO): + headers: dict[str, str] = {} + + def record(rows: list[dict], name: str) -> Path: + destination = tmp_path / name + raw = json.dumps(rows).encode() + module["record_dependency_review"]( + environment, + destination, + opener=lambda request, timeout: ApiResponse(raw), + ) + return destination + + assert json.loads( + (record([added, removed], "valid") / "dependency-diff.json").read_text() + ) == [ + added, + removed, + ] + invalid_cases = [ + ([added, {**removed, "manifest": "uv.lock"}], "unchanged manifest"), + ([added, {**removed, "scope": "runtime"}], "lockfile version/scope"), + ( + [added, removed, {**added, "name": "hono", "version": "4.13.7"}], + "lockfile version/scope", + ), + ] + for index, (rows, error) in enumerate(invalid_cases): + destination = tmp_path / f"invalid-{index}" + with pytest.raises(ValueError, match=error): + record(rows, f"invalid-{index}") + assert not destination.exists() + + +def test_dependency_review_accepts_renamed_lockfile_paths( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + repo = tmp_path / "repo" + repo.mkdir() + subprocess.run(["git", "init", "-q"], cwd=repo, check=True) + subprocess.run(["git", "config", "user.name", "Test"], cwd=repo, check=True) + subprocess.run( + ["git", "config", "user.email", "test@example.com"], cwd=repo, check=True + ) + subprocess.run(["git", "config", "diff.renames", "true"], cwd=repo, check=True) + lock = { + "lockfileVersion": 3, + "packages": {"node_modules/fast-uri": {"version": "3.1.7", "dev": True}}, + } + (repo / "package-lock.json").write_text(json.dumps(lock) + "\n") + subprocess.run(["git", "add", "."], cwd=repo, check=True) + subprocess.run(["git", "commit", "-qm", "base"], cwd=repo, check=True) + base = subprocess.check_output( + ["git", "rev-parse", "HEAD"], cwd=repo, text=True + ).strip() + (repo / "deps").mkdir() + subprocess.run( + ["git", "mv", "package-lock.json", "deps/package-lock.json"], + cwd=repo, + check=True, + ) + subprocess.run(["git", "commit", "-qm", "move lockfile"], cwd=repo, check=True) + head = subprocess.check_output( + ["git", "rev-parse", "HEAD"], cwd=repo, text=True + ).strip() + monkeypatch.chdir(repo) + module = runpy.run_path( + str(WORKFLOW_PATH.parents[2] / "scripts/ci/record_dependency_review.py") + ) + assert module["_changed_files"](base, head) == { + "package-lock.json", + "deps/package-lock.json", + } + removed = { + "change_type": "removed", + "manifest": "package-lock.json", + "ecosystem": "npm", + "name": "fast-uri", + "version": "3.1.7", + "scope": "development", + } + added = {**removed, "change_type": "added", "manifest": "deps/package-lock.json"} + raw = json.dumps([removed, added]).encode() + + class ApiResponse(io.BytesIO): + headers: dict[str, str] = {} + + environment = { + "BASE_SHA": base, + "HEAD_SHA": head, + "GITHUB_REPOSITORY": "groupthinking/EventRelay", + "GITHUB_TOKEN": "dummy-test-token", + "GITHUB_RUN_ID": "123", + "GITHUB_RUN_ATTEMPT": "1", + "REVIEW_OUTCOME": "success", + } + saved = module["record_dependency_review"]( + environment, + tmp_path / "evidence", + opener=lambda request, timeout: ApiResponse(raw), + ) + assert saved["dependency_diff_count"] == 2 + assert (tmp_path / "evidence/dependency-diff.json").read_bytes() == raw