Skip to content

Deacon mp patch 2#3213

Merged
deacon-mp merged 4 commits into
masterfrom
deacon-mp-patch-2
Oct 6, 2025
Merged

Deacon mp patch 2#3213
deacon-mp merged 4 commits into
masterfrom
deacon-mp-patch-2

Conversation

@deacon-mp

Copy link
Copy Markdown
Contributor

Description

(insert summary)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

Updated GitHub Actions workflow to enable SonarQube scans for forked pull requests and adjusted job conditions.
@deacon-mp deacon-mp requested a review from Copilot October 6, 2025 21:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Updates the GitHub Actions quality workflow to support SonarQube scanning for forked pull requests while maintaining security. The changes enable CI/CD to run on PRs from forks by adding a separate job that safely scans forked code.

  • Added pull_request_target trigger for handling forked PRs
  • Created dedicated sonar_fork_pr job for secure SonarQube scanning of forked repositories
  • Improved formatting and comments throughout the workflow

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread .github/workflows/quality.yml Outdated
Comment on lines +112 to +120
with:
args: >
-Dsonar.projectBaseDir=pr
-Dsonar.pullrequest.key=${{ github.event.pull_request.number }}
-Dsonar.pullrequest.branch=${{ github.event.pull_request.head.ref }}
-Dsonar.pullrequest.base=${{ github.event.pull_request.base.ref }}
# Add project/organization explicitly if not in properties file
# -Dsonar.projectKey=<your-project-key>
# -Dsonar.organization=<your-org>

Copilot AI Oct 6, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The with block is misplaced - it should be at the same indentation level as uses and env, not nested under env. This will cause the action to fail as the arguments won't be properly passed to the SonarQube scan.

Copilot uses AI. Check for mistakes.
Updated SonarQube scan action configuration for better project structure.
@sonarqubecloud

sonarqubecloud Bot commented Oct 6, 2025

Copy link
Copy Markdown

1 similar comment
@sonarqubecloud

sonarqubecloud Bot commented Oct 6, 2025

Copy link
Copy Markdown

@deacon-mp deacon-mp merged commit 1bd1815 into master Oct 6, 2025
9 of 10 checks passed
@deacon-mp deacon-mp deleted the deacon-mp-patch-2 branch October 6, 2025 21:54
deacon-mp added a commit that referenced this pull request May 18, 2026
…nar_fork_pr (#3376)

The sonar_fork_pr job in .github/workflows/quality.yml (introduced in #3213)
ran under pull_request_target, checked out the fork's PR HEAD into pr/, then
invoked the Sonar scanner with projectBaseDir pointing at that fork-controlled
directory while SONAR_TOKEN and GITHUB_TOKEN were in the environment.

The scanner resolves sonar-project.properties from projectBaseDir, so a fork
PR could ship its own properties file overriding sonar.host.url and have the
scanner authenticate to an attacker-controlled host — leaking SONAR_TOKEN in
the Authorization header. Reported externally on 2026-05-18.

Fix splits trusted and untrusted work along the GitHub-recommended pattern:

  * .github/workflows/quality.yml
      - drop the pull_request_target trigger entirely
      - drop the sonar_fork_pr job entirely
      - in the existing (secret-less) fork build, upload coverage.xml +
        PR metadata + a source tarball as a 3-day artifact
      - fork-controlled values pass through env vars + jq, never inline
        ${{ }} interpolation into a run: block

  * .github/workflows/sonar-fork-pr.yml  (new)
      - workflow_run trigger (always uses the default-branch definition,
        cannot be replaced by a fork)
      - checks out master (trusted root sonar-project.properties)
      - downloads the fork artifact as DATA, validates pr-meta.json fields
        against strict regexes, cross-checks pr_number and head_sha with
        the GitHub-supplied workflow_run context
      - runs the scanner from the trusted base root with
        sonar.host.url / .organization / .projectKey pinned on the CLI
        (CLI overrides any properties file, belt + suspenders)
      - feeds PR source as -Dsonar.sources only — never as scanner config

SONAR_TOKEN must be rotated in SonarCloud independently of this commit; the
job has been exploitable on public master since 2025-10-06.

Refs: #3213

Co-authored-by: deacon-mp <mperry@mitre.org>
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.

2 participants