diff --git a/.github/workflows/fork-sonar.yml b/.github/workflows/fork-sonar.yml index 2cf829d4..b533efd5 100644 --- a/.github/workflows/fork-sonar.yml +++ b/.github/workflows/fork-sonar.yml @@ -8,6 +8,15 @@ # It runs on `workflow_run` in the base-repo context (with secrets), and is # protected by the `fork-ci` GitHub Environment with Required reviewers. # Do NOT remove that environment gate. +# +# Security model (see docs/CONTRIBUTING.md, "CI for pull requests from forks"): +# nothing fork-controlled is ever *executed* here. Maven only runs against the +# trusted base-branch checkout (to resolve the dependency classpath); the fork +# PR head is then fetched as plain git objects and analysed by the standalone +# Sonar scanner, whose configuration is written by this workflow and passed via +# `project.settings`, so a `sonar-project.properties`, `pom.xml`, `.mvn/`, or +# wrapper script in the fork tree is inert data. Do NOT reintroduce a Maven +# invocation (or any build tool) on the fork tree in this workflow. name: Fork SonarCloud (manual approval) on: @@ -36,8 +45,8 @@ jobs: SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} steps: - name: Download successful fork PR analysis inputs - # Store outside the workspace so the later source checkout (which cleans - # the workspace) does not remove these files. + # Store outside the workspace so the later checkout (which cleans the + # workspace) does not remove these files. uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: name: sonar-analysis-inputs @@ -53,23 +62,25 @@ jobs: run: | env_file="$RUNNER_TEMP/sonar-analysis-data/pr-event.env" pr_number=$(grep -m1 '^pr_number=' "$env_file" | cut -d= -f2-) + pr_head_sha=$(grep -m1 '^pr_head_sha=' "$env_file" | cut -d= -f2-) pr_head_ref=$(grep -m1 '^pr_head_ref=' "$env_file" | cut -d= -f2-) pr_base_ref=$(grep -m1 '^pr_base_ref=' "$env_file" | cut -d= -f2-) [[ "$pr_number" =~ ^[0-9]+$ ]] || { echo "invalid pr_number"; exit 1; } + [[ "$pr_head_sha" =~ ^[0-9a-f]{40}$ ]] || { echo "invalid pr_head_sha"; exit 1; } [[ "$pr_head_ref" =~ ^[A-Za-z0-9._/-]+$ ]] || { echo "invalid pr_head_ref"; exit 1; } [[ "$pr_base_ref" =~ ^[A-Za-z0-9._/-]+$ ]] || { echo "invalid pr_base_ref"; exit 1; } { echo "pr_number=$pr_number" + echo "pr_head_sha=$pr_head_sha" echo "pr_head_ref=$pr_head_ref" echo "pr_base_ref=$pr_base_ref" } >> "$GITHUB_OUTPUT" - - name: Check out fork PR source (from the base repo's PR ref) + - name: Check out trusted base branch + # No `ref`: this is the base repository's default branch, i.e. trusted + # code. It is the only tree Maven ever runs against in this workflow. uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - # The fork head SHA is not present in the base repo, but the PR head - # ref is. Check that out so Sonar can read the analysed source. - ref: refs/pull/${{ steps.meta.outputs.pr_number }}/head fetch-depth: 0 - name: Set up JDK 17 @@ -86,26 +97,79 @@ jobs: key: ${{ runner.os }}-sonar-${{ hashFiles('**/pom.xml') }} restore-keys: ${{ runner.os }}-sonar + - name: Resolve dependency classpath from the trusted base pom + # The Java analyzer needs the dependency jars for full type resolution. + # Resolve them from the base branch's pom (not the fork's) so that the + # only pom Maven parses is trusted. A dependency added by the fork PR + # is simply unresolved in the analysis; it is never downloaded here. + run: | + mvn -B -q -DskipTests -Dfmt.skip -pl core install + mvn -B -q -pl processor dependency:build-classpath \ + -Dmdep.includeScope=test \ + -Dmdep.pathSeparator=, \ + -DexcludeGroupIds=io.github.java-helpers \ + -Dmdep.outputFile="$RUNNER_TEMP/sonar-libraries.txt" + + - name: Fetch fork PR source as git data (not executed) + # The PR head exists in the base repo as refs/pull/N/head. Fetching and + # checking it out only writes files and gives Sonar the SCM history it + # needs to compute changed lines; nothing in the fork tree is run. The + # checked-out commit must be the one the analysis inputs were built from. + env: + PR_NUMBER: ${{ steps.meta.outputs.pr_number }} + PR_HEAD_SHA: ${{ steps.meta.outputs.pr_head_sha }} + run: | + git fetch --no-tags origin "refs/pull/$PR_NUMBER/head" + [ "$(git rev-parse FETCH_HEAD)" = "$PR_HEAD_SHA" ] || { + echo "::error::PR head $(git rev-parse FETCH_HEAD) does not match analysed commit $PR_HEAD_SHA" + exit 1 + } + git -c advice.detachedHead=false checkout --detach FETCH_HEAD + - name: Restore compiled classes and coverage reports run: | src="$RUNNER_TEMP/sonar-analysis-data" mkdir -p core/target/classes processor/target/classes + mkdir -p core/target/test-classes processor/target/test-classes cp -a "$src/core-classes/." core/target/classes/ 2>/dev/null || true cp -a "$src/processor-classes/." processor/target/classes/ 2>/dev/null || true + cp -a "$src/core-test-classes/." core/target/test-classes/ 2>/dev/null || true + cp -a "$src/processor-test-classes/." processor/target/test-classes/ 2>/dev/null || true mkdir -p processor/target/site/jacoco processor/target/site/jacoco-aggregate cp -a "$src/processor-jacoco.xml" processor/target/site/jacoco/jacoco.xml 2>/dev/null || true cp -a "$src/jacoco-aggregate.xml" processor/target/site/jacoco-aggregate/jacoco.xml 2>/dev/null || true + - name: Write scanner configuration + # Written outside the workspace and selected via `project.settings`, so + # a sonar-project.properties in the fork tree is never read. + # Keep the sonar.* values in sync with the in pom.xml. + run: | + libraries="$(cat "$RUNNER_TEMP/sonar-libraries.txt")" + cat > "$RUNNER_TEMP/sonar-project.properties" < + -Dproject.settings=${{ runner.temp }}/sonar-project.properties + -Dsonar.pullrequest.key=${{ steps.meta.outputs.pr_number }} + -Dsonar.pullrequest.branch=${{ steps.meta.outputs.pr_head_ref }} + -Dsonar.pullrequest.base=${{ steps.meta.outputs.pr_base_ref }} diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml index e255791e..2adeec6b 100644 --- a/.github/workflows/maven.yml +++ b/.github/workflows/maven.yml @@ -106,6 +106,8 @@ jobs: mkdir -p sonar-analysis-data cp -a core/target/classes sonar-analysis-data/core-classes 2>/dev/null || true cp -a processor/target/classes sonar-analysis-data/processor-classes 2>/dev/null || true + cp -a core/target/test-classes sonar-analysis-data/core-test-classes 2>/dev/null || true + cp -a processor/target/test-classes sonar-analysis-data/processor-test-classes 2>/dev/null || true cp -a processor/target/site/jacoco-aggregate/jacoco.xml sonar-analysis-data/jacoco-aggregate.xml 2>/dev/null || true cp -a processor/target/site/jacoco/jacoco.xml sonar-analysis-data/processor-jacoco.xml 2>/dev/null || true { diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index 4a0335ed..d76cc883 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -375,6 +375,27 @@ requests from forks, so the secret-backed quality checks are handled specially: run** (in the Actions/Environments prompt) before it executes. It does not rebuild or retest fork code with the token. + The workflow is designed so that **no fork-controlled content is executed** + while the token is present, independent of the approval gate: + - Maven runs only against the trusted base-branch checkout, to resolve the + dependency classpath. The fork's `pom.xml`, `.mvn/` directory and wrapper + scripts are never parsed or run (they can execute arbitrary code, which is + what SonarCloud rule S7631 flags for privileged `workflow_run` jobs). + - The fork PR head is then fetched as plain git data from the base repo's + `refs/pull/N/head`, verified to be the commit the CI artifact was built + from, and checked out so the scanner can read the sources and compute + changed lines from SCM history. + - Analysis runs with the standalone Sonar scanner (`sonarqube-scan-action`), + not the Maven plugin. Its configuration is written by the workflow outside + the workspace and selected via `project.settings`, so a + `sonar-project.properties` in the fork tree is ignored (that file could + otherwise point the scanner at an attacker-supplied Java executable). + + Residual exposure is limited to the scanner *reading* untrusted sources and + bytecode, which is the same exposure as analysing any PR. The `sonar.*` + properties in the workflow mirror those in the root `pom.xml`; keep them in + sync when changing analysis settings. + ## Questions? If you have questions or need help: