ci: harden CI workflows against fork PR privilege abuse

[ricogu]

Summary

Addresses security findings by hardening both codeql.yml and build-and-test.yml against fork PR privilege abuse.

codeql.yml — remove pull_request_target

• Removed pull_request_target — both scanner findings ("no environment protection", "no actor restriction") disappear because the flagged trigger no longer exists
• pull_request now handles all PRs — fires for same-repo branches and fork PRs on public repos
• upload: never for fork PRs — analysis still runs and the job passes/fails on findings, but SARIF is not posted to the base repo's Code Scanning dashboard
• Permissions moved to job level — permissions: {} at workflow level; analyze job declares only contents: read, security-events: write, pull-requests: read
• Simplified checkout — removed explicit ref/repository overrides; default actions/checkout behaviour checks out the PR merge commit, which is the correct analysis target for both same-repo and fork PRs

build-and-test.yml — isolate fork code execution from write-privileged steps

The previous build-dist job ran npm test and npm run build on fork-controlled code while holding contents: write, creating a path where a fork contributor could use GITHUB_TOKEN to push to the base repo. Fixed by splitting the job in two:

• build-test-core (contents: read, persist-credentials: false) — checks out fork code, runs tests and build, uploads the compiled dist/ as a short-lived artifact (retention-days: 1). No write token is available to fork code.
  • For fork PRs: also validates that dist/ is in sync with the source. If not, the step fails and a PR comment is posted explaining what to run locally to fix it (via the isolated comment-dist-dirty job below).
• commit-dist (contents: write, same-repo PRs only) — downloads the pre-built artifact, commits it to the PR branch, then immediately deletes the artifact via gh api DELETE. No fork code executes in this job. Resolves CodeQL CWE-829 alerts by:
  • Using a default actions/checkout (no ref: input) — avoids the "unsafe checkout in trusted context" rule
  • Deriving the PR branch name from git history (HEAD^2 is the PR head; git branch -r --contains finds the remote tracking branch) rather than from any GitHub event/context data — no tainted value flows into the shell
• comment-dist-dirty (pull-requests: write only) — posts a warning callout on the PR when a fork PR's dist/ is out of sync. Isolated into its own job so pull-requests: write is never held by the job that executes untrusted fork code.

Also reduced permissions on all four E2E test jobs (test-python, test-npm, test-maven, test-version-file): contents: write → contents: read (these jobs never write anything) and added persist-credentials: false to their checkouts.

Threat model / abuse path closed

Before: fork contributor opens PR with malicious code in package.json scripts or .github/actions/version-bumping/*/action.yml → build-dist runs npm test/npm run build with contents: write GITHUB_TOKEN available → arbitrary API calls can push branches, modify files, or create releases on the base repo.

After: fork code only executes in build-test-core where the token is contents: read and not stored in the git credential helper. The contents: write job (commit-dist) runs only trusted steps against the pre-built artifact and never runs for fork PRs.

Type of Change

• CI/CD changes

Compatibility Analysis

No behavioural change for same-repo PRs or push/schedule runs. For fork PRs, CodeQL SARIF is no longer uploaded to the base repo's security dashboard (analysis still runs and gates the PR). The build/test pipeline produces the same outputs; only the permission boundaries between jobs change.

[mdanish98]

Hi Rico, following are some of the minor remarks, could you please check if it makes sense?

1. [.github/workflows/build-and-test.yml — commit-dist job] Unpinned third-party action myrotvorets/set-commit-status-action@master

   • Description: The all-tests-passed job uses myrotvorets/set-commit-status-action@master — a floating ref pointing to the latest commit on master. If the upstream repo is compromised or introduces a breaking change, this silently affects the workflow on the next run. This is a supply-chain risk.
   • Suggested fix: Pin to a specific commit SHA (e.g., myrotvorets/set-commit-status-action@3585ea38f3e90f7ec2a7be5e1a2e6f44c0dbff81) and document the version in a comment.

2. [.github/workflows/build-and-test.yml — commit-dist job, line ~145] Branch derivation via git branch -r can silently produce an empty $BRANCH

   • Description: The script uses git branch -r --contains "$PR_HEAD" ... | head -1 to derive the PR branch name. If the remote tracking refs are not populated (e.g., shallow clone edge case or race after force-push), $BRANCH can be empty. The subsequent git checkout -B "$BRANCH" "origin/$BRANCH" would then either fail with a cryptic error or checkout an unexpected ref.
   • Suggested fix: Add an explicit guard:

     if [[ -z "$BRANCH" ]]; then
       echo "::error::Could not determine PR branch from git history."
       exit 1
     fi

3. [.github/workflows/build-and-test.yml — build-test-core job, ref: input] Inline ternary expression reduces readability

   • Description: The ref: input uses a long inline expression:

     ref: ${{ github.event.pull_request.head.repo.full_name == github.repository && github.event.pull_request.head.ref || format('refs/pull/{0}/head', github.event.pull_request.number) }}

     This is functionally correct but hard to audit at a glance. The &&/|| ternary idiom is a common YAML/Actions pattern but is less readable than an explicit if/else in a run: step or a named environment variable.
   • Suggested fix (optional): Extract to a prior run: step that sets a REF env var and use $REF in the checkout step. This makes the logic debuggable in logs. Not blocking.

4. [.github/workflows/build-and-test.yml — comment-dist-dirty job] PR comment deduplication not implemented

   • Description: Each workflow run that fails the dist-dirty check will post a new comment. On repeated pushes to a fork PR that consistently misses the dist/ update, this produces comment spam.
   • Suggested fix (optional): Use gh pr comment --edit-last or check for an existing bot comment before posting. This is a UX concern, not a security or correctness issue.

5. [.github/workflows/codeql.yml — analyze job] SARIF upload condition uses a double-negative expression

   • Description: The upload condition:

     upload: ${{ (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository) && 'never' || 'always' }}

     relies on the same &&/|| ternary pattern and evaluates 'never' for fork PRs. This is correct, but a comment or named step output explaining the intent would make future reviewers more confident the logic is intentional.
   • Suggested fix (optional): Add an inline comment next to the expression, or extract to a preceding step that sets an env var UPLOAD_MODE. Not blocking.

[ricogu]

Thanks @mdanish98! Good catch — pinned myrotvorets/set-commit-status-action to the commit SHA for v2.0.1 (3730c0a348a2ace3c110851bed53331bc6406e9f) in c893e28.

[ricogu]

Addressed the remaining points in 73288bd:

• Added an explicit [[ -z "$BRANCH" ]] guard in commit-dist that fails with a clear error message if the branch cannot be derived from git history.
• comment-dist-dirty now tries --edit-last first so repeated failing pushes update the existing bot comment rather than posting new ones; falls back to a new comment if none exists yet.

[ricogu]

Also addressed 3 and 5 in cf1451d:

• Replaced the ref: ternary in build-test-core with an explicit Resolve checkout ref step that sets CHECKOUT_REF via if/else. The checkout step now reads ${{ env.CHECKOUT_REF }}.
• Replaced the upload: ternary in codeql.yml with a Resolve SARIF upload mode step that sets CODEQL_UPLOAD to never or always. The logic and the intent comment are now co-located in the step rather than inline on the action input.