Skip to content

Stop executing fork-controlled Maven in the privileged fork Sonar workflow (#232) - #285

Merged
AndreasIgel merged 16 commits into
java-helpers:mainfrom
AndreasIgel:devin/1788589722-fork-sonar-232
Sep 12, 2026
Merged

AndreasIgel merged 16 commits into
java-helpers:mainfrom
AndreasIgel:devin/1788589722-fork-sonar-232

Conversation

@AndreasIgel

Copy link
Copy Markdown
Collaborator

Summary

Fixes the SonarCloud S7631 finding on .github/workflows/fork-sonar.yml (new-code Security rating C) by removing every execution of fork-controlled content from the privileged workflow_run job, while keeping the fork-ci required-reviewer gate and the artifact-based design unchanged.

Analysis (issue #232, step 1)

The finding is real, not a false positive: the old workflow checked out refs/pull/N/head and ran mvn ... sonar-maven-plugin:sonar --file pom.xml with SONAR_TOKEN in the environment. Even with -DskipTests, Maven on a fork tree executes code the fork controls:

  • .mvn/extensions.xml / <build><extensions> — arbitrary jars loaded into the Maven process
  • .mvn/maven.config / .mvn/jvm.config — arbitrary flags, e.g. -javaagent:
  • pom.xml plugin bindings — any plugin/execution bound to phases run by the Sonar goal (it forks a build), or <pluginRepositories> pointing at attacker-hosted plugins
  • mvnw wrapper scripts (not used today, but trivially added by a PR)

The required-reviewer gate reduces likelihood but not impact: a maintainer approving a plausible-looking PR cannot review .mvn/ config or plugin coordinates for exfiltration intent. The rule flags the pattern for exactly that reason.

Options considered (step 2)

Option Verdict
(a) Ship sources in the artifact, no checkout at all Sonar PR analysis needs SCM data to compute changed lines / blame; without .git it warns SCM provider autodetection failed and reports 0 new lines, so PR decoration and the new-code gate become meaningless. Also duplicates data already present in refs/pull/N/head. Rejected.
(b) Standalone scanner, no Maven on the fork pom Eliminates all code-execution vectors above; keeps SCM info; fits the "restore, don't rebuild" design. Chosen.
(c) Mark "safe" in SonarCloud Would leave a genuine pwn-request path in place and depend on the human gate alone. Rejected.

Key insight: the problem was never checking out the fork tree (that only writes files), it was running a build tool on it. The fix therefore separates the two.

Implementation (step 3)

fork-sonar.yml now:

checkout            → base repo default branch (trusted), fetch-depth 0
mvn (trusted pom)   → -pl core install; -pl processor dependency:build-classpath
                      (excludes io.github.java-helpers so no stale/self jars)
git fetch           → refs/pull/N/head ; assert FETCH_HEAD == pr_head_sha from artifact
git checkout        → --detach FETCH_HEAD  (files + SCM history only, nothing run)
restore             → classes, test-classes, jacoco xml from artifact
write               → $RUNNER_TEMP/sonar-project.properties (mirrors pom <properties>)
sonarqube-scan-action → -Dproject.settings=$RUNNER_TEMP/... -Dsonar.pullrequest.*

Why each piece matters:

  • Maven only ever sees the trusted pom.xml: it runs before the fork tree exists in the workspace. The classpath it produces is what gives the Java analyzer full type resolution; a dependency added by the fork PR is simply unresolved (analysis degrades, nothing is downloaded on the fork's behalf).
  • pr_head_sha is now validated (^[0-9a-f]{40}$) and asserted against refs/pull/N/head, so the analysed sources match the artifact's bytecode even if the PR was force-pushed between the CI run and the approval.
  • project.settings outside the workspace — with it set, the scanner ignores any sonar-project.properties in the fork tree. This matters because that file can set sonar.scanner.javaExePath (i.e. "run this binary"), which would otherwise be a new fork-controlled execution vector.
  • maven.yml additionally ships core/processor target/test-classes so the standalone scanner gets sonar.java.test.binaries (previously provided implicitly by the Maven plugin).

Sonar properties duplicated from the root pom.xml are called out in a comment and in docs/CONTRIBUTING.md; the docs section now records the security model and this decision.

Validation

  • actionlint clean on both workflows.
  • Classpath resolution command verified locally against main (30 jars, no simple-builders-* self-references).
  • Not yet validated against a real fork PR run — that requires a PR from a fork after this lands on main (workflow_run uses the base-branch workflow file) plus a maintainer approval in the fork-ci environment. Please check the SonarCloud PR decoration shows changed lines and coverage on the first such run.

Closes #232.

AndreasIgel and others added 15 commits August 15, 2026 10:57
The class-level Javadoc and docs/CONFIGURATION.md implied that
@SimpleBuilder is inherited by subclasses, but the annotation was not
meta-annotated with @inherited. As a result BuilderProcessor, which
collects types via RoundEnvironment.getElementsAnnotatedWith(...),
only produced builders for the exact type carrying @SimpleBuilder and
not for unannotated subclasses.

Add @inherited to @SimpleBuilder so subclasses are treated as if they
also carried the annotation, mirroring the existing behaviour of
@SimpleBuilder.Template (which is already @inherited). Update the
Javadoc to document the inheritance explicitly and clarify the
CONFIGURATION.md wording. @Ignore4BuilderGeneration still suppresses
generation for the exact type it is placed on, so opt-outs continue
to work as before.

Add SimpleBuilderInheritanceTest covering direct inheritance, the
opt-out interaction, and multi-level (grandchild) inheritance.

Closes java-helpers#244

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ance

Rename SimpleBuilderInheritanceTest to BuilderAnnotationInheritanceTest
so the name reflects that it covers both builder-triggering annotations.
Add unannotatedSubclassGetsBuilderFromInheritedTemplate, which verifies
that a custom @inherited template annotation (meta-annotated with
@SimpleBuilder.Template) propagates to unannotated subclasses, matching
the existing behaviour of @SimpleBuilder itself.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
The Template Javadoc and CONFIGURATION.md "Template Annotations" section
did not explain that @SimpleBuilder.Template is @inherited, nor that a
custom template annotation must additionally declare @inherited to
propagate to unannotated subclasses. Add explicit documentation and an
example showing the @inherited custom annotation pattern.

Also move assertNoBuilderGenerated to ProcessorAsserts so it is shared
by BuilderAnnotationInheritanceTest and Ignore4BuilderGenerationTest
instead of being duplicated as a private helper in each test class.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
While @SimpleBuilder and @inherited template annotations now correctly
trigger builder generation for unannotated subclasses, the configuration
options declared on the parent's @SimpleBuilder(options = ...) or template
are not yet applied to inherited subclass builders — they use default
options instead. This is tracked separately in issue java-helpers#245.

Add caveats to the SimpleBuilder Javadoc, the CONFIGURATION.md Template
Annotations section, and the Template Annotations Not Working
troubleshooting section so users are not surprised by this limitation.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Add explicit guidance that @SimpleBuilder.Template is a meta-annotation
for custom annotation declarations (@interface) only and cannot be
placed directly on a class or record. @SimpleBuilder is for direct
one-off annotation of classes/records.

- SimpleBuilder.java: add 'When to use' section to class-level Javadoc
- SimpleBuilder.Template Javadoc: state it can only be placed on
  annotation types (ANNOTATION_TYPE), not on classes/records
- CONFIGURATION.md 'Template Annotations': add comparison table and
  introductory paragraph
- CONFIGURATION.md troubleshooting: add item about @SimpleBuilder.Template
  not being a class annotation

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…Javadoc

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…arget

The compiler and IDE already enforce @target(ANNOTATION_TYPE) and show
a clear error when @SimpleBuilder.Template is placed on a class/record,
so this troubleshooting item adds no value.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…kflow (java-helpers#232)

Co-Authored-By: Andreas Igel <andreas.igel@computacenter.com>
@github-actions

Copy link
Copy Markdown
Contributor

Dependency Review

The following issues were found:
  • ✅ 0 vulnerable package(s)
  • ❌ 1 package(s) with incompatible licenses
  • ✅ 0 package(s) with invalid SPDX license definitions
  • ✅ 0 package(s) with unknown licenses.
See the Details below.

Snapshot Warnings

⚠️: No snapshots were found for the head SHA 65704a7.
Ensure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice.

License Issues

.github/workflows/fork-sonar.yml

PackageVersionLicenseIssue Type
SonarSource/sonarqube-scan-action22918119ff8e1ca75a623e15c8296b6ea4fbe28fLGPL-3.0Incompatible License
Allowed Licenses: MIT, Apache-2.0, BSD-2-Clause, BSD-3-Clause, EPL-1.0, EPL-2.0, CDDL-1.0, CDDL-1.1, ISC, Unlicense, CC0-1.0

OpenSSF Scorecard

PackageVersionScoreDetails
actions/SonarSource/sonarqube-scan-action 22918119ff8e1ca75a623e15c8296b6ea4fbe28f 🟢 6
Details
CheckScoreReason
Code-Review🟢 10all changesets reviewed
Maintained🟢 56 commit(s) and 0 issue activity found in the last 90 days -- score normalized to 5
Binary-Artifacts🟢 10no binaries found in the repo
Security-Policy🟢 10security policy file detected
Dangerous-Workflow🟢 10no dangerous workflow patterns detected
Packaging⚠️ -1packaging workflow not detected
Pinned-Dependencies🟢 5dependency not pinned by hash detected -- score normalized to 5
Token-Permissions⚠️ 0detected GitHub workflow tokens with excessive permissions
CII-Best-Practices⚠️ 0no effort to earn an OpenSSF best practices badge detected
License🟢 10license file detected
Fuzzing⚠️ 0project is not fuzzed
Signed-Releases⚠️ -1no releases found
Branch-Protection⚠️ 1branch protection is not maximal on development and all release branches
SAST🟢 8SAST tool is not run on all commits -- score normalized to 8

Scanned Files

  • .github/workflows/fork-sonar.yml

@AndreasIgel
AndreasIgel merged commit 683aa43 into java-helpers:main Sep 12, 2026
5 checks passed
AndreasIgel added a commit that referenced this pull request Sep 12, 2026
Stop executing fork-controlled Maven in the privileged fork Sonar workflow (#232) (#285)

The fork Sonar job ran `mvn sonar:sonar` on the checked-out fork tree
with SONAR_TOKEN set — arbitrary code execution via fork-controlled
pom.xml/.mvn/wrapper in a privileged context.

Now nothing fork-controlled is executed: Maven only runs on the trusted
base checkout (dependency classpath), the fork head is fetched as git
objects and verified against the CI-built SHA, and analysis uses the
standalone sonarqube-scan-action with workflow-written config passed via
project.settings — so fork pom/properties files are inert. The fork-ci
environment gate remains as defense-in-depth.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Analyze fork-sonar.yml untrusted-fork-checkout finding (Sonar S7631) against existing fork-CI security gates

1 participant