fix(security): scope SONAR_TOKEN to scan steps and fail unanalysed fork PRs (#1390, #1392) by don-petry · Pull Request #1417 · combsmith/.github

don-petry · 2026-10-28T03:17:51Z

Summary

Two SonarCloud workflow fixes:

sonarcloud.yml runs the PR's Python test code with the SonarCloud token in the job environment #1392: SONAR_TOKEN no longer shares a job with code from the revision under test.
standards sonarcloud.yml passes the required check when SONAR_TOKEN is unavailable, so fork PRs skip analysis permanently #1390: the required SonarCloud check no longer passes a fork pull request that it could not analyse.

Both fixes are applied to this repo's .github/workflows/sonarcloud.yml and to the standards/workflows/sonarcloud.yml template. A test keeps the token gate script identical in the two files.

Important

Needs a human decision: fork PRs now fail the required SonarCloud check (#1390).
GitHub never passes secrets to fork runs. Once the hole is closed, a fork PR without vars.SONAR_BOOTSTRAP == 'true' cannot get a green SonarCloud check, so contributors see a behaviour change. This PR implements the most conservative secure version, described under #1390 below. The alternatives are listed there too.

#1392: SONAR_TOKEN reachable by the code under test

Fixes #1392

Confirmed. At .github/workflows/sonarcloud.yml:24-25 on main, SONAR_TOKEN was set in the job-level env. The same job ran pip install -r scripts/pytest-requirements.txt (L32) and pytest test/feature-ideation/test_validate_signals.py (L37) on the checked-out PR or merge_group revision, so every step inherited the token. The new guard test/workflows/required-checks/sonarcloud-token-scope.bats fails on main with "job 'sonarcloud' runs repo code and references a secret". The template (standards/workflows/sonarcloud.yml:37-38) used the same job-level env pattern. The template runs no repo code itself, but any build or test step an adopting repo added would inherit the token.

Fix

Coverage moves to a new coverage job (SonarCloud coverage). That job has contents: read, persist-credentials: false and no secrets. It uploads coverage.xml as a 1-day artifact.
The SonarCloud job (needs: coverage) downloads the artifact to the path sonar-project.properties already points at. It runs no repo code.
SONAR_TOKEN now appears only in the step env of the token gate and of the two scan steps, never in a job- or workflow-level env. The same change is made in the template.
A skipped required job reports success. The SonarCloud job therefore runs if: ${{ !cancelled() }} and fails explicitly when needs.coverage.result != 'success'. As before, failing tests still fail SonarCloud.
actions/upload-artifact v7.0.2 and actions/download-artifact v8.0.2 reuse SHAs already pinned in this repo. Both were re-verified with gh api repos/actions/<name>/git/ref/tags/<tag>.

#1390: required SonarCloud check passes fork PRs without analysis

Fixes #1390

Confirmed. In standards/workflows/sonarcloud.yml on main, both scan steps are guarded by if: env.SONAR_TOKEN != '' (L52, L56) and nothing else fails the job. A fork PR receives an empty token, both steps are skipped, and the job reports success to the required SonarCloud context (standards/rulesets/code-quality.json). This repo's own workflow behaved the same way. The new template guard test/scripts/standards-templates/sonarcloud-token-gate.bats fails on main: there is no gate step and the token sits in the job env.

Fix: a Check SonarCloud token gate step. It outputs present=true|false and never echoes the token. Both scan steps now run only when steps.token.outputs.present == 'true'.

Situation	Before	After
Token available	scan	scan (unchanged)
No token, `vars.SONAR_BOOTSTRAP == 'true'`	pass	pass with a `::notice` (new explicit bootstrap opt-in)
No token, fork PR (head repo ≠ base repo, or head repo deleted)	pass	fail with a `::error` that explains why and how a maintainer can get the change analysed
No token, same-repo run (push, same-repo PR, merge queue)	pass	pass with a `::warning` (unchanged)

Why same-repo runs without a token still pass. Per standards/ci-standards.md, SONAR_TOKEN is an org-level secret, so same-repo runs normally have it. The main same-repo run without a token is a Dependabot PR, because Dependabot runs do not receive Actions secrets. Failing that case would block Dependabot auto-merge org-wide. The other case is a genuine day-0 repo. Neither path is reachable by someone without write access.

Decision needed / alternatives

This PR (recommended): fork PRs fail. A maintainer can push the contributor's branch to the repo and open a same-repo PR so the analysis runs, or use the ruleset bypass after review. Re-running the fork PR does not help, because the token is still withheld.
Stricter: also fail same-repo runs without a token unless SONAR_BOOTSTRAP is set. This ends the bootstrap exception for real, but breaks Dependabot PRs unless SONAR_TOKEN is also added as an org Dependabot secret.
Keep status quo for forks: pass with a ::warning. This is non-breaking but leaves standards sonarcloud.yml passes the required check when SONAR_TOKEN is unavailable, so fork PRs skip analysis permanently #1390 open.
Follow-up, larger: a separate credentialed fork-analysis path (workflow_run / pull_request_target that scans without executing fork code). That is a bigger design and is out of scope here.

I did not verify whether vars are exposed to fork pull_request runs. If they are not, SONAR_BOOTSTRAP=true will not rescue fork PRs on a repo that is still bootstrapping, and those PRs fail, which is the fail-closed direction.

Docs

standards/ci-standards.md §3 snippet and prose are updated. The doc now covers: step-scoped token, the separate secret-free coverage job with a !cancelled() guard, and the gate's behaviour table. The doc snippet's older action SHAs are left as they were.

Tests

New bats files. Each one fails on origin/main and passes with this change.

test/scripts/standards-templates/sonarcloud-token-gate.bats (11 tests). It extracts the template's gate run: with yq and executes it for each row of the table above, including deleted head repo, SONAR_BOOTSTRAP other than exactly true, and the token never printed or written to GITHUB_OUTPUT. It also asserts there is no job- or workflow-level SONAR_TOKEN, that only the gate and the scan steps reference the secret, that the scans are gated on present, and that the job name is unchanged. It runs in Standards Templates Tests.
test/workflows/required-checks/sonarcloud-token-scope.bats (10 tests). It covers the same scoping and gate assertions for this repo's workflow. It also checks that the job running pip/pytest references no secrets, that coverage arrives via an artifact, that a failed coverage job fails SonarCloud, and that the gate script is identical to the template's. It runs in Required Checks Tests. required-checks-tests.yml now also triggers on standards/workflows/sonarcloud.yml.
Both rely on yq (mikefarah), which is preinstalled on ubuntu-latest. If yq is missing, they fail loudly instead of skipping.

Local runs:

bats test/workflows/required-checks/ test/scripts/standards-templates/: 52/52 ok. This includes the existing merge-group.bats, which still finds name: SonarCloud.
bats tests/test_emit_standard_file_workflows.bats test/workflows/dependency-hardening.bats test/scripts/deploy-standard-workflows/, run under GNU bash 5: 130/131 ok. The one failure is dry-run.bats "dry-run skips a stub that is already compliant". It is a macOS-only base64 -w incompatibility on add-to-project.yml and is unrelated to this change.
actionlint: no findings in the changed workflows (pre-existing findings elsewhere unchanged). actionlint's shellcheck pass covers the gate script.
yamllint with the ci.yml config (pinned scripts/ci-yamllint-requirements.txt): clean for the changed files.
markdownlint-cli2 standards/ci-standards.md: 0 errors.
shellcheck on the new bats files: clean.

Follow-ups / release notes

Template consumers: sonarcloud.yml is not deployed by deploy-standard-workflows.sh. It ships via combsmith/repo-template, so the template copy there needs a sync PR. Repos that already adopted it keep their copy until updated.
test/feature-ideation/test_validate_signals.py (touched by open PR feat: implement issue #1361 — SonarCloud: code security review (pythonsecurity) #1383) is not modified. Only the job that runs it changed.
Not audited: the scan job still reads the repo-controlled sonar-project.properties. The scanner is not meant to execute repo code, but its scanner-side properties were not reviewed one by one.

🤖 Generated with Claude Code

Problem

See the linked issue.

Risk

Medium — changes GitHub Actions workflow behavior, which is exercised only post-merge; verify via the affected workflow runs.

Test plan

Tests added/updated: test/scripts/standards-templates/sonarcloud-token-gate.bats,test/workflows/required-checks/sonarcloud-token-scope.bats. Verification: bash scripts/dev-lead-lint.sh (shellcheck --severity=warning) ran pre-commit; the bats suite runs in CI.

Rollback

Revert this PR. No non-revertible side effects (no tags, migrations, or external state).

Monitoring

Watch the affected workflow run(s) in the Actions tab and this PR's Lint check for regressions.

chatgpt-codex-connector · 2026-10-28T03:18:04Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

don-petry · 2026-10-28T03:22:28Z

Dev-Lead — waiting on PR blockers (intent: review-changes)

PR: #1417
No changes were committed, but the PR still can't be marked done: required checks CodeQL, SonarCloud are still pending. The retry cron will re-attempt automatically. Next attempt after: 2026-10-28T03:52:26Z

don-petry · 2026-10-28T03:22:29Z

Note

@don-petry I reviewed this PR and no code changes were needed, but I can't mark it done yet: required checks CodeQL, SonarCloud are still pending. I'll re-check automatically.
Next attempt after: 2026-10-28T03:52:26Z

donpetry-bot · 2026-10-28T03:25:21Z

Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-28T04:25:20Z.

donpetry-bot · 2026-10-28T05:26:29Z

No description provided.

don-petry · 2026-10-28T05:28:13Z

Dev-Lead — fix-bot-comment (no-changes)

Agent reasoning

**Issues addressed:** 0 (rate-limit notice only)  
**Disposition:** `informational`
The bot comment contains only a **CodeRabbit free OSS rate-limit notice** — the code review was throttled due to usage limits. No actual code findings, security issues, or actionable items are present in the comment. 
**PR state:**
- All CI checks passing (AGENTS.md check, security scans, SonarCloud, tests, etc.)
- No Tier 1 blockers
- Clean working tree
- Fix commit `5031bce` (scope SONAR_TOKEN to scan steps) is already in place
**Action taken:**
Posted a disposition reply on the original bot issue comment with `disposition=informational`. The maintainer-comment gate will clear the blocking comment once the harness verifies the disposition.

don-petry · 2026-10-28T08:35:32Z

No description provided.

don-petry · 2026-10-28T08:36:42Z

Dev-Lead — review-changes (no-changes)

No changes were needed for this PR.

donpetry-bot · 2026-10-28T11:31:32Z

No description provided.

don-petry · 2026-10-28T11:33:07Z

Code review could not complete — operational notice with no specific findings. No code changes required.

don-petry · 2026-10-28T11:33:19Z

Dev-Lead — fix-bot-comment (no-changes)

Agent reasoning

Issues addressed: 0 (no specific findings)
No code changes required.
The bot comment reports a review failure (operational notice), not code findings. 
All CI checks pass. Dispositioned as informational.
Files changed: none
```
The PR is clear — the bot comment was a failed review attempt with no specific defects identified. I've dispositioned it as `informational` since there are no actionable code findings and all CI checks are passing.

don-petry · 2026-10-28T11:38:14Z

Auto-rebase failed — merge conflict — this branch has conflicts with main that must be resolved.

dev-lead will attempt to resolve this automatically. If it cannot, a follow-up comment will explain what needs manual attention.

To resolve manually instead:

git fetch origin
git merge origin/main
# resolve conflicts, then:
git add .
git commit
git push

…rk PRs (#1390, #1392) job that pip-installs and runs pytest on the checked-out revision. Coverage now runs in a separate `coverage` job with no secrets and hands coverage.xml to the `SonarCloud` job as an artifact. SONAR_TOKEN is in the env of the token gate and the two scan steps only, in both this repo's workflow and the standards template. The SonarCloud job runs when coverage failed (!cancelled) and fails explicitly, so a skipped required job cannot report success. decides the required check: explicit bootstrap (vars.SONAR_BOOTSTRAP == 'true') passes with a notice; a fork pull request fails; a same-repo run (day-0 repo, Dependabot) passes with a warning, as before. Tests: test/scripts/standards-templates/sonarcloud-token-gate.bats and test/workflows/required-checks/sonarcloud-token-scope.bats extract and run the shipped gate script and assert token scoping via yq. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

don-petry · 2026-10-28T11:47:24Z

Dev-Lead — rebase (applied)

Rebase completed and pushed; PR is no longer conflicting.

don-petry · 2026-10-28T11:50:49Z

No description provided.

donpetry-bot · 2026-10-28T11:50:54Z

Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-10-28T12:50:54Z.

don-petry · 2026-11-02T22:15:12Z

@donpetry-bot please review

donpetry-bot · 2026-11-02T22:16:51Z

@don-petry I'm on it — starting a fresh review now. Results will appear in a few minutes.

don-petry · 2026-11-02T23:40:37Z

@donpetry-bot please review

donpetry-bot · 2026-11-02T23:40:49Z

@don-petry I'm on it — starting a fresh review now. Results will appear in a few minutes.

donpetry-bot · 2026-11-03T00:23:06Z

Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-11-03T01:23:06Z.

don-petry · 2026-11-03T00:54:49Z

@donpetry-bot please review

donpetry-bot · 2026-11-03T00:54:59Z

@don-petry I'm on it — starting a fresh review now. Results will appear in a few minutes.

don-petry

Reviewed the workflow token-isolation split (#1392), the fork-PR token gate (#1390), and the new BATS regression suites. The core security fixes and test coverage look solid — only two minor consistency nits noted inline.

…ed step env; docs checkout matches (#1392) The repo-workflow guard only counted references, so a dropped env token plus a stray copy elsewhere still totalled 3. Add the positive check the template's sibling test has. The ci-standards reference snippet now sets persist-credentials: false like the standard it documents. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…1417-merge

don-petry · 2026-11-03T18:34:20Z

@donpetry-bot please review

donpetry-bot · 2026-11-03T19:00:36Z

@don-petry I'm on it — starting a fresh review now. Results will appear in a few minutes.

…1390) pr-review at d241384: the bootstrap check ran before the fork check, so a leftover SONAR_BOOTSTRAP=true let unanalysed fork PRs pass the required check. The fork case now fails first, in both the repo workflow and the template, and bootstrap applies only to same-repo runs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

donpetry-bot · 2026-11-03T19:37:56Z

Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-11-03T20:37:56Z.

don-petry · 2026-11-03T19:38:27Z

@donpetry-bot please review

donpetry-bot · 2026-11-03T19:38:37Z

@don-petry I'm on it — starting a fresh review now. Results will appear in a few minutes.

sonarqubecloud · 2026-11-03T23:49:11Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

donpetry-bot · 2026-11-03T23:53:39Z

Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-11-04T00:53:39Z.

don-petry requested a review from a team as a code owner October 28, 2026 03:17

This comment has been minimized.

Sign in to view

don-petry enabled auto-merge (squash) October 28, 2026 03:22

don-petry disabled auto-merge October 28, 2026 05:27

This comment has been minimized.

Sign in to view

don-petry enabled auto-merge (squash) October 28, 2026 05:28

don-petry disabled auto-merge October 28, 2026 08:36

don-petry enabled auto-merge (squash) October 28, 2026 08:36

don-petry disabled auto-merge October 28, 2026 11:32

don-petry enabled auto-merge (squash) October 28, 2026 11:33

don-petry disabled auto-merge October 28, 2026 11:46

don-petry force-pushed the fix/sec-sonarcloud-token-scope branch from 5031bce to 0e1c996 Compare October 28, 2026 11:47

don-petry enabled auto-merge (squash) October 28, 2026 11:47

don-petry added 2 commits November 2, 2026 17:47

Merge branch 'main' into fix/sec-sonarcloud-token-scope

b9ec78b

Merge branch 'main' into fix/sec-sonarcloud-token-scope

3aaa6c3

don-petry commented Nov 3, 2026

View reviewed changes

Comment thread standards/ci-standards.md Resolved

Comment thread test/workflows/required-checks/sonarcloud-token-scope.bats Resolved

don-petry and others added 6 commits November 2, 2026 21:24

Merge branch 'main' into fix/sec-sonarcloud-token-scope

9697d77

Merge branch 'main' into fix/sec-sonarcloud-token-scope

048c13f

Merge branch 'main' into fix/sec-sonarcloud-token-scope

7e2990b

Merge branch 'main' into fix/sec-sonarcloud-token-scope

a950a84

Merge branch 'main' of https://github.com/combsmith/.github into sec-…

e04e6ab

…1417-merge

don-petry mentioned this pull request Nov 3, 2026

Org Status — 2026-11-03 #1460

Closed

don-petry and others added 2 commits November 3, 2026 13:09

Merge branch 'main' into fix/sec-sonarcloud-token-scope

d241384

Merge branch 'main' into fix/sec-sonarcloud-token-scope

5d9430c

donpetry-bot approved these changes Nov 4, 2026

View reviewed changes

don-petry merged commit 16304fa into main Nov 4, 2026
40 checks passed

don-petry deleted the fix/sec-sonarcloud-token-scope branch November 4, 2026 00:17

Conversation

don-petry commented Oct 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

#1392: SONAR_TOKEN reachable by the code under test

#1390: required SonarCloud check passes fork PRs without analysis

Docs

Tests

Follow-ups / release notes

Problem

Risk

Test plan

Rollback

Monitoring

Uh oh!

This comment has been minimized.

This comment has been minimized.

chatgpt-codex-connector Bot commented Oct 28, 2026

Uh oh!

don-petry commented Oct 28, 2026

Dev-Lead — waiting on PR blockers (intent: review-changes)

Uh oh!

don-petry commented Oct 28, 2026

Uh oh!

donpetry-bot commented Oct 28, 2026

Uh oh!

donpetry-bot commented Oct 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

don-petry commented Oct 28, 2026

Dev-Lead — fix-bot-comment (no-changes)

Uh oh!

don-petry commented Oct 28, 2026

Uh oh!

don-petry commented Oct 28, 2026

Dev-Lead — review-changes (no-changes)

Uh oh!

donpetry-bot commented Oct 28, 2026

Uh oh!

don-petry commented Oct 28, 2026

Uh oh!

don-petry commented Oct 28, 2026

Dev-Lead — fix-bot-comment (no-changes)

Uh oh!

don-petry commented Oct 28, 2026

Uh oh!

don-petry commented Oct 28, 2026

Dev-Lead — rebase (applied)

Uh oh!

don-petry commented Oct 28, 2026

Uh oh!

donpetry-bot commented Oct 28, 2026

Uh oh!

don-petry commented Nov 2, 2026

Uh oh!

donpetry-bot commented Nov 2, 2026

Uh oh!

don-petry commented Nov 2, 2026

Uh oh!

donpetry-bot commented Nov 2, 2026

Uh oh!

donpetry-bot commented Nov 3, 2026

Uh oh!

don-petry commented Nov 3, 2026

Uh oh!

donpetry-bot commented Nov 3, 2026

Uh oh!

don-petry left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

don-petry commented Nov 3, 2026

Uh oh!

donpetry-bot commented Nov 3, 2026

Uh oh!

donpetry-bot commented Nov 3, 2026

Uh oh!

don-petry commented Nov 3, 2026

don-petry commented Oct 28, 2026 •

edited

Loading

donpetry-bot commented Oct 28, 2026 •

edited

Loading