diff --git a/.github/scripts/prepare_fork_coverage_comment.py b/.github/scripts/prepare_fork_coverage_comment.py new file mode 100644 index 00000000..c71dab0a --- /dev/null +++ b/.github/scripts/prepare_fork_coverage_comment.py @@ -0,0 +1,237 @@ +#!/usr/bin/env python3 + +import argparse +import html +import json +import re +import unicodedata +from decimal import Decimal, InvalidOperation +from pathlib import Path +from urllib.parse import parse_qs, urlsplit + +COMMENT_MARKER = "" +EXPECTED_FIELDS = { + "coverage_percentage", + "covered_lines", + "total_lines", + "patch_coverage_pct", + "low_coverage_files", + "ado_url", +} +MAX_JSON_BYTES = 64 * 1024 +MAX_LOW_COVERAGE_BYTES = 8 * 1024 +PERCENTAGE_PATTERN = re.compile(r"^(?:0|[1-9][0-9]{0,2})(?:\.[0-9]+)?%$") +COUNT_PATTERN = re.compile(r"^(?:0|[1-9][0-9]{0,9}|N/A)$") +PATCH_COVERAGE_STATUSES = {"Could not parse", "Report not generated", "N/A%"} +ADO_PROJECT_ID = "904996cc-6198-4d39-8540-eca72bdf0b7b" +ADO_BUILD_PATHS = { + "/sqlclientdrivers/public/_build/results", + f"/sqlclientdrivers/{ADO_PROJECT_ID}/_build/results", +} + + +class ValidationError(ValueError): + pass + + +def _load_json(path: Path, maximum_size: int = MAX_JSON_BYTES): + if not path.is_file() or path.is_symlink(): + raise ValidationError(f"{path.name} must be a regular file") + if path.stat().st_size > maximum_size: + raise ValidationError(f"{path.name} exceeds the {maximum_size}-byte limit") + try: + return json.loads(path.read_text(encoding="utf-8")) + except (UnicodeDecodeError, json.JSONDecodeError) as exc: + raise ValidationError(f"{path.name} is not valid UTF-8 JSON") from exc + + +def _validate_single_line(name: str, value, maximum_length: int) -> str: + if not isinstance(value, str): + raise ValidationError(f"{name} must be a string") + if not value or len(value) > maximum_length: + raise ValidationError(f"{name} has an invalid length") + if any(unicodedata.category(character).startswith("C") for character in value): + raise ValidationError(f"{name} contains control characters") + return value + + +def _validate_percentage(name: str, value, allowed_statuses=frozenset()) -> str: + value = _validate_single_line(name, value, 32) + if value in allowed_statuses: + return value + if not PERCENTAGE_PATTERN.fullmatch(value): + raise ValidationError(f"{name} must be a percentage") + try: + percentage = Decimal(value[:-1]) + except InvalidOperation as exc: + raise ValidationError(f"{name} must be a percentage") from exc + if percentage > 100: + raise ValidationError(f"{name} cannot exceed 100%") + return value + + +def _validate_count(name: str, value) -> str: + value = _validate_single_line(name, value, 16) + if not COUNT_PATTERN.fullmatch(value): + raise ValidationError(f"{name} must be a non-negative integer or N/A") + return value + + +def _validate_low_coverage_files(value) -> str: + if not isinstance(value, str): + raise ValidationError("low_coverage_files must be a string") + if not value or len(value.encode("utf-8")) > MAX_LOW_COVERAGE_BYTES: + raise ValidationError("low_coverage_files has an invalid length") + if "\r" in value: + raise ValidationError("low_coverage_files contains carriage returns") + if len(value.splitlines()) > 10: + raise ValidationError("low_coverage_files contains more than 10 lines") + if any( + character != "\n" and unicodedata.category(character).startswith("C") for character in value + ): + raise ValidationError("low_coverage_files contains control characters") + return value + + +def _validate_ado_url(value) -> str: + value = _validate_single_line("ado_url", value, 500) + parsed = urlsplit(value) + if ( + parsed.scheme != "https" + or parsed.hostname is None + or parsed.hostname.lower() != "dev.azure.com" + or parsed.username is not None + or parsed.password is not None + or parsed.port is not None + or parsed.path.lower() not in ADO_BUILD_PATHS + or parsed.fragment + ): + raise ValidationError("ado_url must reference the public SqlClientDrivers build") + + query = parse_qs(parsed.query, keep_blank_values=True) + if set(query) != {"buildId"}: + raise ValidationError("ado_url contains unexpected query parameters") + build_ids = query.get("buildId", []) + if len(build_ids) != 1 or not build_ids[0].isdigit(): + raise ValidationError("ado_url must contain one numeric buildId") + return ( + f"https://dev.azure.com/SqlClientDrivers/{ADO_PROJECT_ID}/_build/results" + f"?buildId={build_ids[0]}" + ) + + +def validate_artifact(artifact_directory: Path) -> dict: + if not artifact_directory.is_dir() or artifact_directory.is_symlink(): + raise ValidationError("artifact path must be a directory") + + entries = list(artifact_directory.iterdir()) + if ( + len(entries) != 1 + or entries[0].name != "pr-info.json" + or not entries[0].is_file() + or entries[0].is_symlink() + ): + raise ValidationError("artifact must contain only pr-info.json") + + data = _load_json(artifact_directory / "pr-info.json") + if not isinstance(data, dict) or not EXPECTED_FIELDS.issubset(data): + raise ValidationError("pr-info.json does not match the expected schema") + + return { + "coverage_percentage": _validate_percentage( + "coverage_percentage", data["coverage_percentage"] + ), + "covered_lines": _validate_count("covered_lines", data["covered_lines"]), + "total_lines": _validate_count("total_lines", data["total_lines"]), + "patch_coverage_pct": _validate_percentage( + "patch_coverage_pct", + data["patch_coverage_pct"], + PATCH_COVERAGE_STATUSES, + ), + "low_coverage_files": _validate_low_coverage_files(data["low_coverage_files"]), + "ado_url": _validate_ado_url(data["ado_url"]), + } + + +def resolve_pr_number(event: dict, associated_pulls: list) -> int: + repository = event.get("repository", {}) + workflow_run = event.get("workflow_run", {}) + repository_name = repository.get("full_name") + default_branch = repository.get("default_branch") + head_sha = workflow_run.get("head_sha") + head_repository = workflow_run.get("head_repository") or {} + + if not isinstance(repository_name, str) or not repository_name: + raise ValidationError("event is missing the repository name") + if not isinstance(default_branch, str) or not default_branch: + raise ValidationError("event is missing the default branch") + if not isinstance(head_sha, str) or not re.fullmatch(r"[0-9a-f]{40}", head_sha): + raise ValidationError("workflow run has an invalid head SHA") + if head_repository.get("full_name") == repository_name: + raise ValidationError("workflow run did not originate from a fork") + + if not isinstance(associated_pulls, list): + raise ValidationError("associated pull request response must be a list") + matching_pulls = [ + pull + for pull in associated_pulls + if pull.get("head", {}).get("sha") == head_sha + and pull.get("base", {}).get("ref") == default_branch + and pull.get("base", {}).get("repo", {}).get("full_name") == repository_name + and isinstance(pull.get("number"), int) + and pull["number"] > 0 + ] + if len(matching_pulls) != 1: + raise ValidationError("workflow run must resolve to exactly one pull request") + return matching_pulls[0]["number"] + + +def build_comment(data: dict) -> str: + low_coverage_files = html.escape(data["low_coverage_files"]) + ado_url = data["ado_url"] + return f"""\ +{COMMENT_MARKER} +# Code Coverage Report + +| Diff coverage | Overall coverage | Lines covered | +| --- | --- | --- | +| **{data["patch_coverage_pct"]}** | **{data["coverage_percentage"]}** | **{data["covered_lines"]}** of **{data["total_lines"]}** | + +### Files needing attention + +
{low_coverage_files}
+ +[View Azure DevOps build]({ado_url}) +""" + + +def prepare_comment(artifact_directory: Path, event_path: Path, pulls_path: Path): + data = validate_artifact(artifact_directory) + event = _load_json(event_path, 1024 * 1024) + associated_pulls = _load_json(pulls_path, 1024 * 1024) + pr_number = resolve_pr_number(event, associated_pulls) + return pr_number, build_comment(data) + + +def main(): + parser = argparse.ArgumentParser() + parser.add_argument("--artifact-directory", required=True, type=Path) + parser.add_argument("--event", required=True, type=Path) + parser.add_argument("--associated-pulls", required=True, type=Path) + parser.add_argument("--comment-output", required=True, type=Path) + parser.add_argument("--pr-number-output", required=True, type=Path) + args = parser.parse_args() + + try: + pr_number, comment = prepare_comment( + args.artifact_directory, args.event, args.associated_pulls + ) + except ValidationError as exc: + parser.error(str(exc)) + + args.comment_output.write_text(json.dumps({"body": comment}), encoding="utf-8") + args.pr_number_output.write_text(str(pr_number), encoding="ascii") + + +if __name__ == "__main__": + main() diff --git a/.github/workflows/forked-pr-coverage.yml b/.github/workflows/forked-pr-coverage.yml index e616e884..9289f8a9 100644 --- a/.github/workflows/forked-pr-coverage.yml +++ b/.github/workflows/forked-pr-coverage.yml @@ -27,85 +27,65 @@ jobs: runs-on: ubuntu-latest if: > github.event.workflow_run.event == 'pull_request' && - github.event.workflow_run.conclusion == 'success' + github.event.workflow_run.conclusion == 'success' && + github.event.workflow_run.head_repository.full_name != github.repository permissions: + actions: read pull-requests: write contents: read steps: - name: Checkout repo uses: actions/checkout@v4 + with: + persist-credentials: false - - name: Download coverage data + - name: Validate coverage data and post comment env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + RUN_ID: ${{ github.event.workflow_run.id }} run: | - # Download artifact with error handling for non-existent artifacts - if ! gh run download ${{ github.event.workflow_run.id }} \ + set -euo pipefail + + ARTIFACT_DIR="$(mktemp -d "${RUNNER_TEMP}/coverage-comment-data.XXXXXX")" + PULLS_FILE="${RUNNER_TEMP}/associated-pulls.json" + COMMENT_FILE="${RUNNER_TEMP}/coverage-comment.json" + PR_NUMBER_FILE="${RUNNER_TEMP}/coverage-pr-number" + + gh run download "$RUN_ID" \ --repo ${{ github.repository }} \ - --name coverage-comment-data 2>&1; then - echo "⚠️ No coverage-comment-data artifact found" - echo "This is expected for same-repo PRs (they post comments directly)" - echo "Exiting gracefully..." - exit 0 - fi - - # Verify artifact was downloaded - if [[ ! -f pr-info.json ]]; then - echo "⚠️ Artifact downloaded but pr-info.json not found" - echo "This may indicate an issue with artifact upload" - exit 1 - fi + --name coverage-comment-data \ + --dir "$ARTIFACT_DIR" - - name: Read coverage data - id: coverage - run: | - if [[ ! -f pr-info.json ]]; then - echo "❌ pr-info.json not found" - exit 1 - fi - - cat pr-info.json - - # Extract values from JSON with proper quoting - PR_NUMBER="$(jq -r '.pr_number' pr-info.json)" - COVERAGE_PCT="$(jq -r '.coverage_percentage' pr-info.json)" - COVERED_LINES="$(jq -r '.covered_lines' pr-info.json)" - TOTAL_LINES="$(jq -r '.total_lines' pr-info.json)" - PATCH_PCT="$(jq -r '.patch_coverage_pct' pr-info.json)" - LOW_COV_FILES="$(jq -r '.low_coverage_files' pr-info.json)" - PATCH_SUMMARY="$(jq -r '.patch_coverage_summary' pr-info.json)" - ADO_URL="$(jq -r '.ado_url' pr-info.json)" - - # Export to env for next step (single-line values) - echo "PR_NUMBER=${PR_NUMBER}" >> $GITHUB_ENV - echo "COVERAGE_PERCENTAGE=${COVERAGE_PCT}" >> $GITHUB_ENV - echo "COVERED_LINES=${COVERED_LINES}" >> $GITHUB_ENV - echo "TOTAL_LINES=${TOTAL_LINES}" >> $GITHUB_ENV - echo "PATCH_COVERAGE_PCT=${PATCH_PCT}" >> $GITHUB_ENV - echo "ADO_URL=${ADO_URL}" >> $GITHUB_ENV - - # Handle multiline values with proper quoting - { - echo "LOW_COVERAGE_FILES<> $GITHUB_ENV - - { - echo "PATCH_COVERAGE_SUMMARY<> $GITHUB_ENV + gh api --paginate --slurp \ + -H "Accept: application/vnd.github+json" \ + "repos/${GITHUB_REPOSITORY}/pulls?state=open&per_page=100" \ + --jq 'add' > "$PULLS_FILE" - - name: Comment coverage summary on PR - uses: ./.github/actions/post-coverage-comment - with: - pr_number: ${{ env.PR_NUMBER }} - coverage_percentage: ${{ env.COVERAGE_PERCENTAGE }} - covered_lines: ${{ env.COVERED_LINES }} - total_lines: ${{ env.TOTAL_LINES }} - patch_coverage_pct: ${{ env.PATCH_COVERAGE_PCT }} - low_coverage_files: ${{ env.LOW_COVERAGE_FILES }} - patch_coverage_summary: ${{ env.PATCH_COVERAGE_SUMMARY }} - ado_url: ${{ env.ADO_URL }} + python .github/scripts/prepare_fork_coverage_comment.py \ + --artifact-directory "$ARTIFACT_DIR" \ + --event "$GITHUB_EVENT_PATH" \ + --associated-pulls "$PULLS_FILE" \ + --comment-output "$COMMENT_FILE" \ + --pr-number-output "$PR_NUMBER_FILE" + + PR_NUMBER="$(cat "$PR_NUMBER_FILE")" + + COMMENT_ID="$( + gh api --paginate --slurp \ + "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" \ + --jq 'add | map(select( + .user.login == "github-actions[bot]" and + (.body | contains("")) + )) | .[0].id // empty' + )" + + if [[ -n "$COMMENT_ID" ]]; then + gh api --method PATCH \ + "repos/${GITHUB_REPOSITORY}/issues/comments/${COMMENT_ID}" \ + --input "$COMMENT_FILE" > /dev/null + else + gh api --method POST \ + "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" \ + --input "$COMMENT_FILE" > /dev/null + fi diff --git a/.github/workflows/pr-code-coverage.yml b/.github/workflows/pr-code-coverage.yml index c07204f3..6de471ed 100644 --- a/.github/workflows/pr-code-coverage.yml +++ b/.github/workflows/pr-code-coverage.yml @@ -17,6 +17,7 @@ jobs: uses: actions/checkout@v4 with: fetch-depth: 0 + persist-credentials: false - name: Setup git for diff-cover run: | @@ -58,6 +59,15 @@ jobs: RESULT=$(echo "$BUILD_INFO" | jq -r '.result // "unknown"') BUILD_ID=$(echo "$BUILD_INFO" | jq -r '.id // "unknown"') WEB_URL=$(echo "$BUILD_INFO" | jq -r '._links.web.href // "unknown"') + + [[ "$BUILD_ID" =~ ^[0-9]+$ ]] || { + echo "Invalid Azure DevOps build ID" + exit 1 + } + if [[ ${#WEB_URL} -gt 500 || "$WEB_URL" == *$'\n'* || "$WEB_URL" == *$'\r'* ]]; then + echo "Invalid Azure DevOps build URL" + exit 1 + fi echo "✅ Found build: ID=$BUILD_ID, Status=$STATUS, Result=$RESULT" echo "🔗 Build URL: $WEB_URL" @@ -204,9 +214,17 @@ jobs: sort -t: -k2 -n | head -10) fi - echo "LOW_COVERAGE_FILES<> $GITHUB_ENV - echo "${LOW_COVERAGE_FILES:-No detailed file data available}" >> $GITHUB_ENV - echo "EOF" >> $GITHUB_ENV + LOW_COVERAGE_FILES="${LOW_COVERAGE_FILES:-No detailed file data available}" + LOW_COVERAGE_DELIMITER="coverage_$(openssl rand -hex 16)" + if grep -Fxq "$LOW_COVERAGE_DELIMITER" <<< "$LOW_COVERAGE_FILES"; then + echo "Generated delimiter unexpectedly occurs in coverage data" + exit 1 + fi + { + echo "LOW_COVERAGE_FILES<<$LOW_COVERAGE_DELIMITER" + echo "$LOW_COVERAGE_FILES" + echo "$LOW_COVERAGE_DELIMITER" + } >> "$GITHUB_ENV" echo "✅ Coverage data extracted successfully" else @@ -372,6 +390,10 @@ jobs: PATCH_COVERAGE=$(jq -r '.total_percent_covered // "N/A"' patch-coverage.json) TOTAL_STATEMENTS=$(jq -r '.total_num_lines // "N/A"' patch-coverage.json) MISSING_STATEMENTS=$(jq -r '.total_num_missing // "N/A"' patch-coverage.json) + + if [[ ! "$PATCH_COVERAGE" =~ ^[0-9]+([.][0-9]+)?$ ]]; then + PATCH_COVERAGE="N/A" + fi echo "✅ Patch coverage: ${PATCH_COVERAGE}%" echo "📊 Total lines: $TOTAL_STATEMENTS, Missing: $MISSING_STATEMENTS" @@ -408,9 +430,16 @@ jobs: # Extract summary for comment if [[ -f patch-coverage.md ]]; then - echo "PATCH_COVERAGE_SUMMARY<> $GITHUB_ENV - cat patch-coverage.md >> $GITHUB_ENV - echo "EOF" >> $GITHUB_ENV + PATCH_COVERAGE_DELIMITER="coverage_$(openssl rand -hex 16)" + if grep -Fxq "$PATCH_COVERAGE_DELIMITER" patch-coverage.md; then + echo "Generated delimiter unexpectedly occurs in patch coverage data" + exit 1 + fi + { + echo "PATCH_COVERAGE_SUMMARY<<$PATCH_COVERAGE_DELIMITER" + cat patch-coverage.md + echo "$PATCH_COVERAGE_DELIMITER" + } >> "$GITHUB_ENV" echo "✅ Patch coverage markdown summary ready" else echo "⚠️ patch-coverage.md not generated" @@ -421,22 +450,18 @@ jobs: run: | mkdir -p coverage-comment-data jq -n \ - --arg pr_number "${{ github.event.pull_request.number }}" \ --arg coverage_percentage "${{ env.COVERAGE_PERCENTAGE }}" \ --arg covered_lines "${{ env.COVERED_LINES }}" \ --arg total_lines "${{ env.TOTAL_LINES }}" \ --arg patch_coverage_pct "${{ env.PATCH_COVERAGE_PCT }}" \ --arg low_coverage_files "$LOW_COVERAGE_FILES" \ - --arg patch_coverage_summary "$PATCH_COVERAGE_SUMMARY" \ --arg ado_url "${{ env.ADO_URL }}" \ '{ - pr_number: $pr_number, coverage_percentage: $coverage_percentage, covered_lines: $covered_lines, total_lines: $total_lines, patch_coverage_pct: $patch_coverage_pct, low_coverage_files: $low_coverage_files, - patch_coverage_summary: $patch_coverage_summary, ado_url: $ado_url }' > coverage-comment-data/pr-info.json diff --git a/tests/test_fork_coverage_security.py b/tests/test_fork_coverage_security.py new file mode 100644 index 00000000..dc85afd6 --- /dev/null +++ b/tests/test_fork_coverage_security.py @@ -0,0 +1,206 @@ +import importlib.util +import json +from pathlib import Path + +import pytest + +SCRIPT_PATH = Path(__file__).parents[1] / ".github" / "scripts" / "prepare_fork_coverage_comment.py" +SPEC = importlib.util.spec_from_file_location("prepare_fork_coverage_comment", SCRIPT_PATH) +coverage_comment = importlib.util.module_from_spec(SPEC) +SPEC.loader.exec_module(coverage_comment) +REPOSITORY_ROOT = Path(__file__).parents[1] + + +@pytest.fixture +def valid_artifact(tmp_path): + artifact = tmp_path / "artifact" + artifact.mkdir() + data = { + "coverage_percentage": "81.5%", + "covered_lines": "815", + "total_lines": "1000", + "patch_coverage_pct": "92%", + "low_coverage_files": "mssql_python/example.py: 55.0%", + "ado_url": ( + "https://dev.azure.com/SqlClientDrivers/" + "904996cc-6198-4d39-8540-eca72bdf0b7b/_build/results?buildId=46466" + ), + } + (artifact / "pr-info.json").write_text(json.dumps(data), encoding="utf-8") + return artifact + + +def _event(pull_requests=None): + return { + "repository": { + "full_name": "microsoft/mssql-python", + "default_branch": "main", + }, + "workflow_run": { + "head_sha": "a" * 40, + "head_repository": {"full_name": "contributor/mssql-python"}, + "pull_requests": pull_requests or [], + }, + } + + +def test_prepares_safe_comment_for_forked_pr(valid_artifact, tmp_path): + event_path = tmp_path / "event.json" + pulls_path = tmp_path / "pulls.json" + event_path.write_text(json.dumps(_event()), encoding="utf-8") + pulls_path.write_text( + json.dumps( + [ + { + "number": 123, + "head": {"sha": "a" * 40}, + "base": { + "ref": "main", + "repo": {"full_name": "microsoft/mssql-python"}, + }, + } + ] + ), + encoding="utf-8", + ) + + pr_number, body = coverage_comment.prepare_comment(valid_artifact, event_path, pulls_path) + + assert pr_number == 123 + assert coverage_comment.COMMENT_MARKER in body + assert "buildId=46466" in body + assert coverage_comment.ADO_PROJECT_ID in body + + +def test_ignores_event_supplied_pull_requests(): + associated_pulls = [ + { + "number": 456, + "head": {"sha": "a" * 40}, + "base": { + "ref": "main", + "repo": {"full_name": "microsoft/mssql-python"}, + }, + } + ] + + # An attacker-controlled workflow_run.pull_requests entry must never + # short-circuit resolution; only the trusted head SHA match is honored. + resolved = coverage_comment.resolve_pr_number(_event([{"number": 999}]), associated_pulls) + + assert resolved == 456 + + +def test_ignores_artifact_supplied_pr_number(valid_artifact): + path = valid_artifact / "pr-info.json" + data = json.loads(path.read_text(encoding="utf-8")) + data["pr_number"] = "999" + path.write_text(json.dumps(data), encoding="utf-8") + + validated = coverage_comment.validate_artifact(valid_artifact) + + # Extra fields are tolerated but never propagated; the PR number is + # sourced only from the trusted workflow event, never from the artifact. + assert "pr_number" not in validated + + +def test_rejects_missing_required_field(valid_artifact): + path = valid_artifact / "pr-info.json" + data = json.loads(path.read_text(encoding="utf-8")) + del data["ado_url"] + path.write_text(json.dumps(data), encoding="utf-8") + + with pytest.raises(coverage_comment.ValidationError, match="expected schema"): + coverage_comment.validate_artifact(valid_artifact) + + +def test_rejects_unexpected_artifact_payload(valid_artifact): + (valid_artifact / "payload.so").write_bytes(b"not executable") + + with pytest.raises(coverage_comment.ValidationError, match="only pr-info.json"): + coverage_comment.validate_artifact(valid_artifact) + + +def test_rejects_unexpected_artifact_directory(valid_artifact): + (valid_artifact / "nested" / "empty").mkdir(parents=True) + + with pytest.raises(coverage_comment.ValidationError, match="only pr-info.json"): + coverage_comment.validate_artifact(valid_artifact) + + +def test_escapes_multiline_file_data_in_comment(valid_artifact): + path = valid_artifact / "pr-info.json" + data = json.loads(path.read_text(encoding="utf-8")) + data["low_coverage_files"] = "safe.py: 50%\n" + path.write_text(json.dumps(data), encoding="utf-8") + + validated = coverage_comment.validate_artifact(valid_artifact) + body = coverage_comment.build_comment(validated) + + assert "