ci: harden the cloud test path and pin the remaining floating actions

[thomasboni]

Summary

Five focused changes to the workflows, each in its own commit. No test or documentation impact.

1. The cloud test job runs pull request code with a write-all token (run-tests-cloud.yml)

claude-code-review.yml already carries this comment:

SECURITY: pull_request_target checks out the base branch by default. Do NOT add ref: ${{ github.event.pull_request.head.sha }} here, that would run untrusted fork code with access to repository secrets.

run-tests-cloud.yml does that. Under pull_request_target it checks out the head SHA, and the workflow grants write-all, so the job that executes fork code holds a full write token alongside the cloud credentials, on a self-hosted runner.

The label gate is the right control and stays untouched. What changes is what the opted-in code can reach: the workflow default drops to contents: read, the one step that needed a write scope (removing the trigger label) moves to its own job on a hosted runner that never checks out the PR, and the checkout stops persisting the git credential. Behaviour is unchanged, the label is still removed after every run including failures.

This does not remove the underlying pull_request_target plus head-checkout pattern, because that is what makes cloud testing of a PR work at all. Splitting the credential-bearing part from the fork-code part would be the full fix, and it is your architectural call rather than something to slip into this PR. Happy to open a follow-up if you want it.

2. Three more workflows default to write-all (run-tests.yml, run-tests-head.yml, release.yml)

None of them uses GITHUB_TOKEN for anything that writes. The two test workflows check out and run tests with no API calls, and release.yml mints a GitHub App token and uses it for the checkout, the release API call and the push, so the workflow token is unused. All three become contents: read.

3. Five action references were not pinned

lycheeverse/lychee-action@v2, anthropics/claude-code-action@v1, golangci/golangci-lint-action@v9 and two actions/checkout@main references now pin a commit with the version in a trailing comment. Two are worth calling out: the CodeQL steps were on v1, a tag that last moved in January 2023, so the analysis has been running a three-year-old revision of the action. They move to v4.37.6. And actions/checkout@main tracks a moving branch rather than a release.

Left alone deliberately: the two ClickHouse/integrations-shared-workflows references. They are your own shared workflows and pinning them would stop central updates reaching this repo, which is presumably the point. Same reasoning for the clickhouse/docs-builder image. Happy to pin either if you would rather have them fixed.

4. The link checker had no permissions: block, so its token fell back to the repository default. It checks out and runs a link checker with no API calls, so contents: read.

5. The benchmark job passed secrets: inherit (stresshouse-benchmark-compare.yml), handing the called workflow every secret in the repository. clickhouse-benchmark.yml declares exactly two, WORKFLOW_AUTH_PUBLIC_APP_ID and WORKFLOW_AUTH_PUBLIC_PRIVATE_KEY. Passing those explicitly keeps the cloud credentials out of a workflow that does not ask for them, and means a secret added later is not forwarded by default.

One thing worth knowing if you use a repository-level Actions allowlist: entries written as tag patterns (owner/action@v1) stop matching once a workflow pins a SHA, and every workflow using that action then fails at startup. The fix is owner/action@* in settings.

Found and fixed by Plumber's analysis, reviewed and submitted by me.

[CLAassistant]

All committers have signed the CLA.

[kavirajk]

Thanks @thomasboni for the PR.

Two things

1. Left few comments to clarify
2. Can you sign the CLA?

[kavirajk]

why not inherit the secrets here?

[thomasboni]

secrets: inherit forwards every secret into the workflow, while the callee here only declares WORKFLOW_AUTH_PUBLIC_APP_ID and WORKFLOW_AUTH_PUBLIC_PRIVATE_KEY, so everything beyond those two is exposure with no benefit.

And this particular callee lives in a different repo (ClickHouse/integrations-shared-workflows) and is referenced on @main, meaning any future change to that workflow immediately runs here with whatever secrets we hand it.

Passing them explicitly caps what a change on that side can ever reach, and doubles as documentation of what the workflow actually needs. If the shared workflow grows a new secret later, the failure is loud (a missing-secret error) instead of a silent broadening of access.

[kavirajk]

I'm not super comfortable with git commit hash here. Can't we just do @v9.3.0 here? (similar in other places?)

[thomasboni]

In most cases the git commit hash is the only way to be sure the action you are using is immutable. It's highly recommended when the action comes from an external source that could be compromised and run malicious code in your pipeline (which has access to secrets, code, envs, etc.). That's the route the tj-actions/changed-files compromise took.

But in this specific case with golangci/golangci-lint-action you are right to prefer @v9.3.0, because the repo uses GitHub's immutable releases. The v9.3.0 release is marked immutable, meaning the tag can never be repointed or deleted. So @v9.3.0 gives the same guarantee as the SHA, and I'm going to switch that one back. Same for codeql-action, its releases are immutable too.

The others are different:

• lychee-action@v2.9.0: mutable tag from a third-party org. I'd keep the SHA here, but if you prefer the tag for readability I can revert it.
• claude-code-action@v1.0.190: mutable tag, maintained by Anthropic. Larger vendor, but still a third party with a repointable tag. Your call, tell me which way you want it.
• actions/checkout@v5.1.0: mutable tag, maintained by GitHub itself. If you trust GitHub the tag is reasonable, I will revert to @v5.1.0 (tell me if not).

[github-actions]

⚠️ Should fix — remove-label job now fires on every labeled event

always() overrides the skipped test dependency, so this job runs on every labeled event on every PR — the old step only ran when the trigger label was tests:run-cloud, because it lived inside test behind that job's if gate.

• Adding any other label (e.g. review:claude-agent) while a cloud run is in flight deletes tests:run-cloud mid-run.
• Every unrelated label event now spawns a runner job just to attempt a no-op DELETE.

Suggested change

	if: always() && github.event_name == 'pull_request_target'
	if: always() && github.event_name == 'pull_request_target' && github.event.label.name == 'tests:run-cloud'

[thomasboni]

Right, always() was meant to guarantee cleanup when the test job fails but as written it also fires for unrelated labels. Restored the label guard on the job condition.

[github-actions]

⚠️ Should fix — highest-risk job still uses tag-pinned actions

This job runs untrusted fork code next to the cloud credentials on a self-hosted runner — the highest-value target in the repo — yet its own actions (actions/checkout@v4, actions/setup-go@v5) stay on mutable tags while lower-risk workflows got SHA pins. Tag repointing is exactly the vector the rest of this PR defends against.

• The same applies to actions/checkout@v5 in check-pr-links.yml, actions/checkout@v4 in the other test workflows, and actions/create-github-app-token@v1 in release.yml, which handles the App private key.
• GitHub-owned actions are lower-risk, but if tag trust were sufficient the five newly pinned refs could have kept their tags too — one pinning policy, applied consistently, is easier to audit.

[thomasboni]

I disagree with the bot here: even if it doesn't look consistent, we should keep the easy human-readable tag except for risky inclusion where we have to use the SHA.

[github-actions]

🤖 Claude review

Hardens the CI workflows: four write-all defaults drop to contents: read, the cloud-test label removal moves to its own minimal-scope job on a hosted runner, the fork-code checkout stops persisting credentials, five action refs get SHA pins, and the benchmark caller passes two named secrets instead of secrets: inherit. The direction is right and most of the PR's claims check out against the workflow files, but two items should be fixed and the pins need verification before merge.

Verified against the workflow files:

• release.yml really doesn't use the workflow token — checkout, push, and the release API call all use the App token — so contents: read is safe.
• run-tests.yml, run-tests-head.yml, and check-pr-links.yml make no token writes; contents: read is sufficient.
• codeql-analysis.yml already grants security-events: write at the job level, so the v1 → v4 bump has the permissions it needs (v1 is deprecated upstream, so this is also a functional fix).
• The new remove-label job's issues: write scope and secrets.GITHUB_TOKEN are correct for the label-delete API call, and needs: [test] + always() preserves removal after failures.

Key concerns:

• The new remove-label job runs on every labeled event, not just tests:run-cloud — a real behavior change despite the PR's "behaviour is unchanged" claim (inline, one-line fix).
• The cloud-test job — fork code plus cloud secrets on a self-hosted runner — still consumes actions/checkout and actions/setup-go by mutable tag while lower-risk workflows got SHA pins (inline).

Blind spots: no network access from the review environment, so I could not (a) resolve the five pinned SHAs to their claimed version tags or (b) fetch integrations-shared-workflows/clickhouse-benchmark.yml to confirm it declares exactly the two secrets now passed explicitly. Both need a quick maintainer check before merge (general findings).

Verdict: ⚠️ Request changes

General findings

• ⚠️ Should fix — verify each pinned SHA resolves to its annotated tag before merge
  The five new pins are only as good as the SHA↔tag mapping, and a wrong pin is worse than a floating tag because nothing will ever move it. This review environment has no network access, so the mapping could not be independently verified. Before merging, confirm each pin, e.g.:

  git ls-remote --tags https://github.com/actions/checkout v5.1.0
  git ls-remote --tags https://github.com/github/codeql-action v4.37.6
  git ls-remote --tags https://github.com/lycheeverse/lychee-action v2.9.0
  git ls-remote --tags https://github.com/anthropics/claude-code-action v1.0.190
  git ls-remote --tags https://github.com/golangci/golangci-lint-action v9.3.0
  

  and check the (peeled ^{}) SHA matches the trailing comment in each workflow.

• ⚠️ Should fix — confirm the shared benchmark workflow's secret contract
  The switch from secrets: inherit to explicit passing in stresshouse-benchmark-compare.yml assumes clickhouse-benchmark.yml in ClickHouse/integrations-shared-workflows declares exactly WORKFLOW_AUTH_PUBLIC_APP_ID and WORKFLOW_AUTH_PUBLIC_PRIVATE_KEY. That repo was not reachable from the review environment. If the callee declares a third secret — even an optional one — this change silently drops whatever feature uses it; and since the callee is referenced @main, a later addition of a required secret will break this caller at startup. Worth a maintainer check, plus a note in the shared repo that callers pass secrets explicitly.

_{Inline comments are attached to the relevant lines. This summary updates in place on re-review.}

[thomasboni]

Hi @kavirajk

• CLA is signed
• Let me know if my answers are clear :)